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

[Russell King (Oracle)]

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.

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