Re: [PATCH] cciss: return 0 from driver probe function on success,not 1

[Tomas Henzl]

On 10/31/2013 11:06 PM, Andrew Morton wrote:
> On Thu, 31 Oct 2013 16:54:44 -0500 scameron@xxxxxxxxxxxxxxxxxx wrote:
>
>>>> How did this ever work?
>>> Beats me.  local_pci_probe() does
>>>
>>> 	rc = pci_drv->probe(pci_dev, ddi->id);
>>> 	if (rc) {
>>> 		pci_dev->driver = NULL;
>>> 		pm_runtime_put_sync(dev);
>>> 	}
>>> 	return rc;
>>>
>>> shrug, maybe this ->probe somehow has a different caller which checks
>>> for <0.
>> Older kernels (eg: http://lxr.linux.no/#linux+v2.6.32.61/drivers/pci/pci-driver.c )
>> had different code:
>>
>>  330__pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
>>  331{
>>  332        const struct pci_device_id *id;
>>  333        int error = 0;
>>  334
>>  335        if (!pci_dev->driver && drv->probe) {
>>  336                error = -ENODEV;
>>  337
>>  338                id = pci_match_device(drv, pci_dev);
>>  339                if (id)
>>  340                        error = pci_call_probe(drv, pci_dev, id);
>>  341                if (error >= 0) {
>>  342                        pci_dev->driver = drv;
>>  343                        error = 0;
>>  344                }
>>  345        }
>>  346        return error;
>>  347}
> So cciss is presently kompletely kaput?  If so, the kapputting code is
> present in 3.9 and probably earlier, so this patch is needed in 3.12 and
> -stable.  Or if not, what?

Steve posted similar patch for hpsa too and I'm sure someone would notice
when the hpsa driver was kaput. If I understand it correctly the driver
works with some limitation - rmmod doesn't work for example.

The problem in kernel is that the error handling in local_pci_probe
and  in __pci_device_probe is different for ret values > 0,
so we should fix it somewhere so it is in sync.
The documentation states that the probe function should return zero on success
so what about this -

This would bring the handling to sync
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 98f7b9b..200a071 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -317,8 +317,6 @@ __pci_device_probe(struct pci_driver *drv, struct pci_dev *pci_dev)
 		id = pci_match_device(drv, pci_dev);
 		if (id)
 			error = pci_call_probe(drv, pci_dev, id);
-		if (error >= 0)
-			error = 0;
 	}
 	return error;
 }


>
> (Playing question and answer like this is a bad way of writing a
> changelog btw - all this stuff should have been right there in the v1
> changelog).
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/