[Devel] Re: [PATCH 5/5] fixing errors handling during pci_driver resume stage [serial]

[Dmitriy Monakhov]

Russell King <rmk+lkml at arm.linux.org.uk> writes:

> On Tue, Jan 09, 2007 at 12:01:58PM +0300, Dmitriy Monakhov wrote:
>> serial pci drivers have to return correct error code during resume stage in
>> case of errors.
>
> Sigh.  *hate* *hate* *hate*.
>
>> diff --git a/drivers/serial/8250_pci.c b/drivers/serial/8250_pci.c
>> index 52e2e64..e26e4a6 100644
>> --- a/drivers/serial/8250_pci.c
>> +++ b/drivers/serial/8250_pci.c
>> @@ -1805,6 +1805,7 @@ static int pciserial_suspend_one(struct
>>  static int pciserial_resume_one(struct pci_dev *dev)
>>  {
>>  	struct serial_private *priv = pci_get_drvdata(dev);
>> +	int err;
>>  
>>  	pci_set_power_state(dev, PCI_D0);
>>  	pci_restore_state(dev);
>> @@ -1813,7 +1814,12 @@ static int pciserial_resume_one(struct p
>>  		/*
>>  		 * The device may have been disabled.  Re-enable it.
>>  		 */
>> -		pci_enable_device(dev);
>> +		err = pci_enable_device(dev);
>> +		if (err) {
>> +			dev_err(&dev->dev, "Cannot enable PCI device, "
>> +				"aborting.\n");
>> +			return err;
>> +		}
>>  
>>  		pciserial_resume_ports(priv);
>>  	}
>
> So if pci_enable_device() fails, what do we do with the still suspended
> serial port?  Does it clean up that state?  Probably not.
>
> Look, merely going around bunging this stupid "oh lets propagate the
> error" crap into the kernel doesn't actually fix _anything_.  In fact
> it potentially _hides_ the warnings produced by __must_check which
> give a hint that _something_ needs to be done to _properly_ fix the
> problem.
>
> And by "properly", I mean not just merely propagating the error.
>
> In this particular case, the above may result in resources not being
> freed.
Yep 100% true. But the question is _HOW_? We want shutdown not enabled device.
Is it safe just call pciserial_remove_ports() for this device? 
>
> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/