Re: [PATCH net-next 06/16] net: pcs: xpcs: Avoid creating dummy XPCS MDIO device

[Serge Semin]

On Wed, Dec 13, 2023 at 04:32:21PM +0000, Russell King (Oracle) wrote:
> On Wed, Dec 13, 2023 at 03:01:45AM +0300, Serge Semin wrote:
> > On Tue, Dec 05, 2023 at 01:46:44PM +0000, Russell King (Oracle) wrote:
> > > xpcs_create_mdiodev() as it originally stood creates the mdiodev from
> > > the bus/address, and then passes that to xpcs_create(). Once
> > > xpcs_create() has finished its work (irrespective of whether it was
> > > successful or not) we're done with the mdiodev in this function, so
> > > the reference is _always_ put.
> > 
> > You say that it's required to manage the refcounting twice: when we
> > get the reference from some external place and internally when the
> > reference is stored in the XPCS descriptor. What's the point in such
> > redundancy with the internal ref-counting if we know that the pointer
> > can be safely stored and utilized afterwards? Better maintainability?
> > Is it due to having the object retrieval and storing implemented in
> > different functions?
> 
> The point is that the error handling gets simpler:
> - One can see in xpcs_create_mdiodev() that the reference taken by
>   mdio_device_create() is always dropped if that function was
>   successful, irrespective of whether xpcs_create() was successful.
> 
> - xpcs_create() is responsible for managing the refcount on the mdiodev
>   that is passed to it - and if it's successful, it needs to increment
>   the refcount, or leave it in the same state as it was on entry if
>   failing.
> 
> This avoids complexities in error paths, which are notorious for things
> being forgotten - since with this, each of these functions is resposible
> for managing its refcount.
> 
> It's a different style of refcount management, one I think more people
> should adopt.
> 
> > While at it if you happen to know an answer could you please also
> > clarify the next question. None of the ordinary
> > platform/PCI/USB/hwmon/etc drivers I've been working with managed
> > refcounting on storing a passed to probe() device pointer in the
> > private driver data. Is it wrong not doing that?
> 
> If we wanted to do refcounting strictly, then every time a new
> pointer to a data structure is created, we should be taking a refcount
> on it, and each time that pointer is destroyed, we should be putting
> the refcount. That is what refcounting is all about.
> 
> However, there are circumstances where this can be done lazily, and
> for drivers we would prefer driver authors not to end up with
> refcount errors where they've forgotten to put something.
> 
> In the specific case of drivers, we have a well defined lifetime for
> a device bound to a driver. We guarantee that the struct device will
> not go away if a driver is bound to the device, until such time that
> the driver's .remove method has been called. Thus, we guarantee that
> the device driver will be notified of the struct device going away
> before it has been freed. This frees the driver author from having
> to worry about the refcount of the struct device.
> 
> As soon as we start doing stuff that is outside of that model, then
> objects that are refcounted need to be dealt with, and I much prefer
> the "strict" refcounting implementation such as the one I added to
> xpcs, because IMHO it's much easier to see that the flow is obviously
> correct - even if it does need a comment to describe why we always
> do a put.

Ok. I fully get your point now: lazy refcounting for the drivers
following standard model and the 'strict' one for others. It sounds
reasonable. I'll get that adopted in my future developments. Thank you
very much for the detailed explanation and for all your comments.

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!