| Message ID | E1q2USm-008PAO-Gx@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c4933fa88a68c69205753601044949d516c4db10 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: pcs: add helpers to xpcs and lynx to manage mdiodev | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 568 this patch: 568 |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/build_clang | success | Errors and warnings before: 295 this patch: 295 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 538 this patch: 538 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti: > Add two new operations for a mdio device to manage the refcount on the > underlying struct device. This will be used by mdio PCS drivers to > simplify the creation and destruction handling, making it easier for > users to get it correct. ... > +static inline void mdio_device_get(struct mdio_device *mdiodev) > +{ > + get_device(&mdiodev->dev); Dunno what is the practice in net, but it makes sense to have the pattern as if (mdiodev) return to_mdiodev(get_device(&mdiodev->dev)); which might be helpful in some cases. This approach is used in many cases in the kernel. > +} P.S. Just my 2c, you may ignore the above comment.
On Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) wrote: > Add two new operations for a mdio device to manage the refcount on the > underlying struct device. This will be used by mdio PCS drivers to > simplify the creation and destruction handling, making it easier for > users to get it correct. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Fri, May 26, 2023 at 09:20:17PM +0300, andy.shevchenko@gmail.com wrote: > Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti: > > Add two new operations for a mdio device to manage the refcount on the > > underlying struct device. This will be used by mdio PCS drivers to > > simplify the creation and destruction handling, making it easier for > > users to get it correct. > > ... > > > +static inline void mdio_device_get(struct mdio_device *mdiodev) > > +{ > > + get_device(&mdiodev->dev); > > Dunno what is the practice in net, but it makes sense to have the pattern as > > if (mdiodev) > return to_mdiodev(get_device(&mdiodev->dev)); > > which might be helpful in some cases. This approach is used in many cases in > the kernel. The device should exist. If it does not, it is a bug, and the resulting opps will help find the bug. Andrew
On Mon, May 29, 2023 at 6:21 PM Andrew Lunn <andrew@lunn.ch> wrote: > On Fri, May 26, 2023 at 09:20:17PM +0300, andy.shevchenko@gmail.com wrote: > > Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti: ... > > > +static inline void mdio_device_get(struct mdio_device *mdiodev) > > > +{ > > > + get_device(&mdiodev->dev); > > > > Dunno what is the practice in net, but it makes sense to have the pattern as > > > > if (mdiodev) > > return to_mdiodev(get_device(&mdiodev->dev)); > > > > which might be helpful in some cases. This approach is used in many cases in > > the kernel. > > The device should exist. If it does not, it is a bug, and the > resulting opps will help find the bug. The main point in the returned value, the 'if' can be dropped, it's not a big deal.
On Mon, May 29, 2023 at 11:34:42PM +0300, Andy Shevchenko wrote: > On Mon, May 29, 2023 at 6:21 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Fri, May 26, 2023 at 09:20:17PM +0300, andy.shevchenko@gmail.com wrote: > > > Fri, May 26, 2023 at 11:14:24AM +0100, Russell King (Oracle) kirjoitti: > > ... > > > > > +static inline void mdio_device_get(struct mdio_device *mdiodev) > > > > +{ > > > > + get_device(&mdiodev->dev); > > > > > > Dunno what is the practice in net, but it makes sense to have the pattern as > > > > > > if (mdiodev) > > > return to_mdiodev(get_device(&mdiodev->dev)); > > > > > > which might be helpful in some cases. This approach is used in many cases in > > > the kernel. > > > > The device should exist. If it does not, it is a bug, and the > > resulting opps will help find the bug. > > The main point in the returned value, the 'if' can be dropped, it's > not a big deal. And if you do, that results in a latent bug. The whole point of doing the above is to cater for the case when mdiodev is NULL. If mdiodev is NULL, provided "dev" is the first member of mdiodev, then &mdiodev->dev will also be NULL. That will mean get_device() will see a NULL pointer and itself return NULL. The to_mdiodev() will then convert back to a mdiodev which will also be NULL. So everything works correctly. If dev is not the first member, then &mdiodev->dev will no longer be NULL, but will be offset by that amount. get_device() will check whether that is NULL - it won't be, so it'll try to pass &dev->kobj to kobject_get(). That will also not be a NULL pointer. kobject_get() will likely oops the kernel. So no, it isn't safe to drop that if(). However, count the number of places in the kernel that actually pay attention to the return value of get_device()... In drivers/base, which should be the prime area where things are done correctly, there are 42 places where get_device() is called. Of those 42 places, 13 places make use of the returned value in some way, which mean 29 do not bother to check. Now, what use is checking that return value? Does get_device() ever return anything different from what was passed in? Well, we need to delve further down into kobject_get(), which does not - kobject_get() returns whatever it was given. Note that kref_get() doesn't return anything, nor does refcount_inc(), both of which post-date get_device(). So, that in turn means that get_device() will only ever return what it was passed. So: ret = get_device(dev); `ret' is _guaranteed_ to always be exactly the same was `dev' in all cases (except if "dev" results in get_device() performing an invalid dereference and Oopsing the kernel, by which time we won't care about `ret'.) Now, if we think about situations where this will be called - they will always be called where the MDIO device already has reference counts against it - we have to be holding at least one refcount to already have a reference to this device. If we don't have that, then we're in the situation where the memory pointed to by the mdio device pointer has probably already been freed, and could already be used for some other purpose - and using the return value from get_device() isn't going to save you from such a racy stupidity. If we extend this to mdio_device_get(), then we end up in exactly the same scenario - and what benefit does it give to the code? Does it make the code more readable? No, it makes it less readable. Does it make the code more robust? No, it makes no difference. Does it make the code more correct? Again, no difference. So, I don't see any point in changing the proposal I've put forward as mdio_device_get(). Things would be different if get_device() used kobject_get_unless_zero() which _can_ alter the returned value, but as I've stated above, if the refcount were to become zero at the point that mdio_device_get() may be called, we've *already* lost.
diff --git a/include/linux/mdio.h b/include/linux/mdio.h index 0670cc6e067c..c1b7008826e5 100644 --- a/include/linux/mdio.h +++ b/include/linux/mdio.h @@ -106,6 +106,16 @@ int mdio_driver_register(struct mdio_driver *drv); void mdio_driver_unregister(struct mdio_driver *drv); int mdio_device_bus_match(struct device *dev, struct device_driver *drv); +static inline void mdio_device_get(struct mdio_device *mdiodev) +{ + get_device(&mdiodev->dev); +} + +static inline void mdio_device_put(struct mdio_device *mdiodev) +{ + mdio_device_free(mdiodev); +} + static inline bool mdio_phy_id_is_c45(int phy_id) { return (phy_id & MDIO_PHY_ID_C45) && !(phy_id & ~MDIO_PHY_ID_C45_MASK);
Add two new operations for a mdio device to manage the refcount on the underlying struct device. This will be used by mdio PCS drivers to simplify the creation and destruction handling, making it easier for users to get it correct. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- include/linux/mdio.h | 10 ++++++++++ 1 file changed, 10 insertions(+)