Re: Can we drop __must_check from driver_for_each_device()?
On Wed, 2013-08-07 at 14:51 +0900, Greg Kroah-Hartman wrote: > On Tue, Aug 06, 2013 at 10:31:25PM +0200, Paul Bolle wrote: > > On Fri, 2013-08-02 at 08:31 +0800, Greg Kroah-Hartman wrote: > > > On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote: > > > > 2) Please note that if the callback always returns zero, > > > > driver_for_each_device() can still return -EINVAL, but only if it was > > > > provided a NULL "drv" (a struct device_driver). It sure seems odd to do > > > > so. Can that actually happen? > > > > > > Possibly. > > > > So driver_for_each_device() really should be NULL "drv" safe. > > Probably not, now that I think about it some more. I don't see how that > could ever really happen, do you? No, I don't. See there are two groups of user of driver_for_each_device(): 1) those that call it on the device_driver member of their platform_driver, pci_driver, or usb_driver. If that device_driver member turns out to be NULL we've got bigger problems, I guess; 2) those that call driver_find() to get a pointer to a device_driver. These users can trivially check for NULL before calling driver_for_each_device(). [Side note: the comment above driver_find() reads in part: This routine provides no locking to prevent the driver it returns from being unregistered or unloaded while the caller is using it. The caller is responsible for preventing this. None of these callers of driver_find() seem to be troubled by this. Their use of the pointer to a device_driver could be racey.] Anyhow, I think the NULL "dev" check can be dropped. > > But wouldn't it therefor be better to make sure the callback functions > > do not return -EINVAL themselves, so -EINVAL will always only indicate > > the NULL "drv" case? > > I doubt it's a real need at all. This is, of course, why I raised this issue. Because if we drop the NULL "drv" check, we have a function that will in most cases always return zero, because the callbacks it uses mostly return zero. So I think that if the NULL "drv" check could be dropped, we might as well drop __must_check here. The few cases were the callback might return non-zero are already handled correctly. > > So, since this warning is still there, I'm looking for another way to > > get rid of it. > > Fix it properly would be good to do, don't you think? It's hard to disagree with that! Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Can we drop __must_check from driver_for_each_device()?
On Wed, 2013-08-07 at 14:51 +0900, Greg Kroah-Hartman wrote: On Tue, Aug 06, 2013 at 10:31:25PM +0200, Paul Bolle wrote: On Fri, 2013-08-02 at 08:31 +0800, Greg Kroah-Hartman wrote: On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote: 2) Please note that if the callback always returns zero, driver_for_each_device() can still return -EINVAL, but only if it was provided a NULL drv (a struct device_driver). It sure seems odd to do so. Can that actually happen? Possibly. So driver_for_each_device() really should be NULL drv safe. Probably not, now that I think about it some more. I don't see how that could ever really happen, do you? No, I don't. See there are two groups of user of driver_for_each_device(): 1) those that call it on the device_driver member of their platform_driver, pci_driver, or usb_driver. If that device_driver member turns out to be NULL we've got bigger problems, I guess; 2) those that call driver_find() to get a pointer to a device_driver. These users can trivially check for NULL before calling driver_for_each_device(). [Side note: the comment above driver_find() reads in part: This routine provides no locking to prevent the driver it returns from being unregistered or unloaded while the caller is using it. The caller is responsible for preventing this. None of these callers of driver_find() seem to be troubled by this. Their use of the pointer to a device_driver could be racey.] Anyhow, I think the NULL dev check can be dropped. But wouldn't it therefor be better to make sure the callback functions do not return -EINVAL themselves, so -EINVAL will always only indicate the NULL drv case? I doubt it's a real need at all. This is, of course, why I raised this issue. Because if we drop the NULL drv check, we have a function that will in most cases always return zero, because the callbacks it uses mostly return zero. So I think that if the NULL drv check could be dropped, we might as well drop __must_check here. The few cases were the callback might return non-zero are already handled correctly. So, since this warning is still there, I'm looking for another way to get rid of it. Fix it properly would be good to do, don't you think? It's hard to disagree with that! Thanks, Paul Bolle -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Can we drop __must_check from driver_for_each_device()?
On Tue, Aug 06, 2013 at 10:31:25PM +0200, Paul Bolle wrote: > On Fri, 2013-08-02 at 08:31 +0800, Greg Kroah-Hartman wrote: > > On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote: > > > 2) Please note that if the callback always returns zero, > > > driver_for_each_device() can still return -EINVAL, but only if it was > > > provided a NULL "drv" (a struct device_driver). It sure seems odd to do > > > so. Can that actually happen? > > > > Possibly. > > So driver_for_each_device() really should be NULL "drv" safe. Probably not, now that I think about it some more. I don't see how that could ever really happen, do you? > But wouldn't it therefor be better to make sure the callback functions > do not return -EINVAL themselves, so -EINVAL will always only indicate > the NULL "drv" case? I doubt it's a real need at all. > > > 3) So to me it looks the __must_check attribute of > > > driver_for_each_device() can be dropped. Do you agree? > > > > Nope, it should be making people think about the return value of the > > function. If they use it or not might be a problem, but I would argue > > that those call-sites must be fixed, as you point out above. > > I see. I guess I should try to submit patches that do just that. > > > Is this somehow causing a problem that removing the marking would solve > > for you? > > The, rather trivial, issue I'd like to fix is this (long standing) > warning: > drivers/isdn/hardware/mISDN/hfcpci.c:2298:2: warning: \ > ignoring return value of ‘driver_for_each_device’, \ > declared with attribute warn_unused_result [-Wunused-result] > > I've submitted a patch to silence that warning about a year ago (see > https://lkml.org/lkml/2012/9/21/138 ). Dave Miller was pretty clear that > that approach wouldn't do. (I've added Dave to the CC, just because I > mentioned him here.) I agree with David, that patch is pointless. > So, since this warning is still there, I'm looking for another way to > get rid of it. Fix it properly would be good to do, don't you think? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Can we drop __must_check from driver_for_each_device()?
On Fri, 2013-08-02 at 08:31 +0800, Greg Kroah-Hartman wrote: > On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote: > > 2) Please note that if the callback always returns zero, > > driver_for_each_device() can still return -EINVAL, but only if it was > > provided a NULL "drv" (a struct device_driver). It sure seems odd to do > > so. Can that actually happen? > > Possibly. So driver_for_each_device() really should be NULL "drv" safe. But wouldn't it therefor be better to make sure the callback functions do not return -EINVAL themselves, so -EINVAL will always only indicate the NULL "drv" case? > > 3) So to me it looks the __must_check attribute of > > driver_for_each_device() can be dropped. Do you agree? > > Nope, it should be making people think about the return value of the > function. If they use it or not might be a problem, but I would argue > that those call-sites must be fixed, as you point out above. I see. I guess I should try to submit patches that do just that. > Is this somehow causing a problem that removing the marking would solve > for you? The, rather trivial, issue I'd like to fix is this (long standing) warning: drivers/isdn/hardware/mISDN/hfcpci.c:2298:2: warning: \ ignoring return value of ‘driver_for_each_device’, \ declared with attribute warn_unused_result [-Wunused-result] I've submitted a patch to silence that warning about a year ago (see https://lkml.org/lkml/2012/9/21/138 ). Dave Miller was pretty clear that that approach wouldn't do. (I've added Dave to the CC, just because I mentioned him here.) So, since this warning is still there, I'm looking for another way to get rid of it. Thanks! Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Can we drop __must_check from driver_for_each_device()?
On Fri, 2013-08-02 at 08:31 +0800, Greg Kroah-Hartman wrote: On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote: 2) Please note that if the callback always returns zero, driver_for_each_device() can still return -EINVAL, but only if it was provided a NULL drv (a struct device_driver). It sure seems odd to do so. Can that actually happen? Possibly. So driver_for_each_device() really should be NULL drv safe. But wouldn't it therefor be better to make sure the callback functions do not return -EINVAL themselves, so -EINVAL will always only indicate the NULL drv case? 3) So to me it looks the __must_check attribute of driver_for_each_device() can be dropped. Do you agree? Nope, it should be making people think about the return value of the function. If they use it or not might be a problem, but I would argue that those call-sites must be fixed, as you point out above. I see. I guess I should try to submit patches that do just that. Is this somehow causing a problem that removing the marking would solve for you? The, rather trivial, issue I'd like to fix is this (long standing) warning: drivers/isdn/hardware/mISDN/hfcpci.c:2298:2: warning: \ ignoring return value of ‘driver_for_each_device’, \ declared with attribute warn_unused_result [-Wunused-result] I've submitted a patch to silence that warning about a year ago (see https://lkml.org/lkml/2012/9/21/138 ). Dave Miller was pretty clear that that approach wouldn't do. (I've added Dave to the CC, just because I mentioned him here.) So, since this warning is still there, I'm looking for another way to get rid of it. Thanks! Paul Bolle -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Can we drop __must_check from driver_for_each_device()?
On Tue, Aug 06, 2013 at 10:31:25PM +0200, Paul Bolle wrote: On Fri, 2013-08-02 at 08:31 +0800, Greg Kroah-Hartman wrote: On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote: 2) Please note that if the callback always returns zero, driver_for_each_device() can still return -EINVAL, but only if it was provided a NULL drv (a struct device_driver). It sure seems odd to do so. Can that actually happen? Possibly. So driver_for_each_device() really should be NULL drv safe. Probably not, now that I think about it some more. I don't see how that could ever really happen, do you? But wouldn't it therefor be better to make sure the callback functions do not return -EINVAL themselves, so -EINVAL will always only indicate the NULL drv case? I doubt it's a real need at all. 3) So to me it looks the __must_check attribute of driver_for_each_device() can be dropped. Do you agree? Nope, it should be making people think about the return value of the function. If they use it or not might be a problem, but I would argue that those call-sites must be fixed, as you point out above. I see. I guess I should try to submit patches that do just that. Is this somehow causing a problem that removing the marking would solve for you? The, rather trivial, issue I'd like to fix is this (long standing) warning: drivers/isdn/hardware/mISDN/hfcpci.c:2298:2: warning: \ ignoring return value of ‘driver_for_each_device’, \ declared with attribute warn_unused_result [-Wunused-result] I've submitted a patch to silence that warning about a year ago (see https://lkml.org/lkml/2012/9/21/138 ). Dave Miller was pretty clear that that approach wouldn't do. (I've added Dave to the CC, just because I mentioned him here.) I agree with David, that patch is pointless. So, since this warning is still there, I'm looking for another way to get rid of it. Fix it properly would be good to do, don't you think? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Can we drop __must_check from driver_for_each_device()?
On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote:
> Greg,
>
> 0) Summary: I think __must_check can be dropped from
> driver_for_each_device(). Do you agree?
No.
> 1) Commit 4a7fb6363f ("add __must_check to device management code")
> added __must_check to driver_for_each_device(), seven years ago. But of
> the seventeen current users of that function just one actually seems to
> care about its return value:
>
> - dead code:
> drivers/mtd/onenand/omap2.c:omap2_onenand_rephase()
>
> - callback always returns zero:
> drivers/infiniband/hw/qib/qib_init.c:qib_notify_dca()
> (see all three possible values of f_notify_dca())
> drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_audio_fini()
> drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_alsa_init()
> drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_exit()
> drivers/net/ethernet/intel/igb/igb_main.c:igb_notify_dca()
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_notify_dca()
> drivers/net/ethernet/myricom/myri10ge/myri10ge.c:myri10ge_notify_dca()
>
> - return value of callback ignored:
> drivers/iommu/omap-iommu.c:omap_foreach_iommu_device()
> (when called by iommu_debugfs_exit())
> drivers/isdn/hardware/mISDN/hfcpci.c:hfcpci_softirq()
> drivers/media/pci/cx18/cx18-alsa-main.c:cx18_alsa_exit()
> drivers/media/pci/ivtv/ivtv-alsa-main.c:ivtv_alsa_exit()
> drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_init()
> drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_cleanup()
> drivers/media/platform/s5p-tv/mixer_video.c:find_and_register_subdev()
> drivers/net/wireless/brcm80211/brcmfmac/usb.c:brcmf_usb_exit()
>
> - return value actually used:
> drivers/iommu/omap-iommu.c:omap_foreach_iommu_device()
> (when called by iommu_debug_init())
>
> 2) Please note that if the callback always returns zero,
> driver_for_each_device() can still return -EINVAL, but only if it was
> provided a NULL "drv" (a struct device_driver). It sure seems odd to do
> so. Can that actually happen?
Possibly.
> 3) So to me it looks the __must_check attribute of
> driver_for_each_device() can be dropped. Do you agree?
Nope, it should be making people think about the return value of the
function. If they use it or not might be a problem, but I would argue
that those call-sites must be fixed, as you point out above.
Is this somehow causing a problem that removing the marking would solve
for you?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Can we drop __must_check from driver_for_each_device()?
On Wed, Jul 31, 2013 at 11:35:13PM +0200, Paul Bolle wrote: Greg, 0) Summary: I think __must_check can be dropped from driver_for_each_device(). Do you agree? No. 1) Commit 4a7fb6363f (add __must_check to device management code) added __must_check to driver_for_each_device(), seven years ago. But of the seventeen current users of that function just one actually seems to care about its return value: - dead code: drivers/mtd/onenand/omap2.c:omap2_onenand_rephase() - callback always returns zero: drivers/infiniband/hw/qib/qib_init.c:qib_notify_dca() (see all three possible values of f_notify_dca()) drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_audio_fini() drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_alsa_init() drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_exit() drivers/net/ethernet/intel/igb/igb_main.c:igb_notify_dca() drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_notify_dca() drivers/net/ethernet/myricom/myri10ge/myri10ge.c:myri10ge_notify_dca() - return value of callback ignored: drivers/iommu/omap-iommu.c:omap_foreach_iommu_device() (when called by iommu_debugfs_exit()) drivers/isdn/hardware/mISDN/hfcpci.c:hfcpci_softirq() drivers/media/pci/cx18/cx18-alsa-main.c:cx18_alsa_exit() drivers/media/pci/ivtv/ivtv-alsa-main.c:ivtv_alsa_exit() drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_init() drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_cleanup() drivers/media/platform/s5p-tv/mixer_video.c:find_and_register_subdev() drivers/net/wireless/brcm80211/brcmfmac/usb.c:brcmf_usb_exit() - return value actually used: drivers/iommu/omap-iommu.c:omap_foreach_iommu_device() (when called by iommu_debug_init()) 2) Please note that if the callback always returns zero, driver_for_each_device() can still return -EINVAL, but only if it was provided a NULL drv (a struct device_driver). It sure seems odd to do so. Can that actually happen? Possibly. 3) So to me it looks the __must_check attribute of driver_for_each_device() can be dropped. Do you agree? Nope, it should be making people think about the return value of the function. If they use it or not might be a problem, but I would argue that those call-sites must be fixed, as you point out above. Is this somehow causing a problem that removing the marking would solve for you? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Can we drop __must_check from driver_for_each_device()?
Greg,
0) Summary: I think __must_check can be dropped from
driver_for_each_device(). Do you agree?
1) Commit 4a7fb6363f ("add __must_check to device management code")
added __must_check to driver_for_each_device(), seven years ago. But of
the seventeen current users of that function just one actually seems to
care about its return value:
- dead code:
drivers/mtd/onenand/omap2.c:omap2_onenand_rephase()
- callback always returns zero:
drivers/infiniband/hw/qib/qib_init.c:qib_notify_dca()
(see all three possible values of f_notify_dca())
drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_audio_fini()
drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_alsa_init()
drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_exit()
drivers/net/ethernet/intel/igb/igb_main.c:igb_notify_dca()
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_notify_dca()
drivers/net/ethernet/myricom/myri10ge/myri10ge.c:myri10ge_notify_dca()
- return value of callback ignored:
drivers/iommu/omap-iommu.c:omap_foreach_iommu_device()
(when called by iommu_debugfs_exit())
drivers/isdn/hardware/mISDN/hfcpci.c:hfcpci_softirq()
drivers/media/pci/cx18/cx18-alsa-main.c:cx18_alsa_exit()
drivers/media/pci/ivtv/ivtv-alsa-main.c:ivtv_alsa_exit()
drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_init()
drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_cleanup()
drivers/media/platform/s5p-tv/mixer_video.c:find_and_register_subdev()
drivers/net/wireless/brcm80211/brcmfmac/usb.c:brcmf_usb_exit()
- return value actually used:
drivers/iommu/omap-iommu.c:omap_foreach_iommu_device()
(when called by iommu_debug_init())
2) Please note that if the callback always returns zero,
driver_for_each_device() can still return -EINVAL, but only if it was
provided a NULL "drv" (a struct device_driver). It sure seems odd to do
so. Can that actually happen?
3) So to me it looks the __must_check attribute of
driver_for_each_device() can be dropped. Do you agree?
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Can we drop __must_check from driver_for_each_device()?
Greg, 0) Summary: I think __must_check can be dropped from driver_for_each_device(). Do you agree? 1) Commit 4a7fb6363f (add __must_check to device management code) added __must_check to driver_for_each_device(), seven years ago. But of the seventeen current users of that function just one actually seems to care about its return value: - dead code: drivers/mtd/onenand/omap2.c:omap2_onenand_rephase() - callback always returns zero: drivers/infiniband/hw/qib/qib_init.c:qib_notify_dca() (see all three possible values of f_notify_dca()) drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_audio_fini() drivers/media/pci/cx25821/cx25821-alsa.c:cx25821_alsa_init() drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_exit() drivers/net/ethernet/intel/igb/igb_main.c:igb_notify_dca() drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_notify_dca() drivers/net/ethernet/myricom/myri10ge/myri10ge.c:myri10ge_notify_dca() - return value of callback ignored: drivers/iommu/omap-iommu.c:omap_foreach_iommu_device() (when called by iommu_debugfs_exit()) drivers/isdn/hardware/mISDN/hfcpci.c:hfcpci_softirq() drivers/media/pci/cx18/cx18-alsa-main.c:cx18_alsa_exit() drivers/media/pci/ivtv/ivtv-alsa-main.c:ivtv_alsa_exit() drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_init() drivers/media/pci/ivtv/ivtvfb.c:ivtvfb_cleanup() drivers/media/platform/s5p-tv/mixer_video.c:find_and_register_subdev() drivers/net/wireless/brcm80211/brcmfmac/usb.c:brcmf_usb_exit() - return value actually used: drivers/iommu/omap-iommu.c:omap_foreach_iommu_device() (when called by iommu_debug_init()) 2) Please note that if the callback always returns zero, driver_for_each_device() can still return -EINVAL, but only if it was provided a NULL drv (a struct device_driver). It sure seems odd to do so. Can that actually happen? 3) So to me it looks the __must_check attribute of driver_for_each_device() can be dropped. Do you agree? Paul Bolle -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
