* Can we drop __must_check from driver_for_each_device()? @ 2013-07-31 21:35 Paul Bolle 2013-08-02 0:31 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Paul Bolle @ 2013-07-31 21:35 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Can we drop __must_check from driver_for_each_device()? 2013-07-31 21:35 Can we drop __must_check from driver_for_each_device()? Paul Bolle @ 2013-08-02 0:31 ` Greg Kroah-Hartman 2013-08-06 20:31 ` Paul Bolle 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2013-08-02 0:31 UTC (permalink / raw) To: Paul Bolle; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Can we drop __must_check from driver_for_each_device()? 2013-08-02 0:31 ` Greg Kroah-Hartman @ 2013-08-06 20:31 ` Paul Bolle 2013-08-07 5:51 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Paul Bolle @ 2013-08-06 20:31 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: David Miller, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Can we drop __must_check from driver_for_each_device()? 2013-08-06 20:31 ` Paul Bolle @ 2013-08-07 5:51 ` Greg Kroah-Hartman 2013-08-07 16:57 ` Paul Bolle 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2013-08-07 5:51 UTC (permalink / raw) To: Paul Bolle; +Cc: David Miller, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Can we drop __must_check from driver_for_each_device()? 2013-08-07 5:51 ` Greg Kroah-Hartman @ 2013-08-07 16:57 ` Paul Bolle 0 siblings, 0 replies; 5+ messages in thread From: Paul Bolle @ 2013-08-07 16:57 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: David Miller, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-08-07 16:58 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-31 21:35 Can we drop __must_check from driver_for_each_device()? Paul Bolle 2013-08-02 0:31 ` Greg Kroah-Hartman 2013-08-06 20:31 ` Paul Bolle 2013-08-07 5:51 ` Greg Kroah-Hartman 2013-08-07 16:57 ` Paul Bolle
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.