Re: [RFC PATCH v2 linux-next 13/14] net: dsa: sja1105: expose the SGMII PCS as an mdio_device

On Wed, May 26, 2021 at 04:46:41PM +0100, Russell King (Oracle) wrote:

On Wed, May 26, 2021 at 06:41:02PM +0300, Vladimir Oltean wrote:

quoted

On Wed, May 26, 2021 at 04:29:11PM +0100, Russell King (Oracle) wrote:

quoted

On Wed, May 26, 2021 at 04:55:34PM +0300, Vladimir Oltean wrote:

quoted

Since we touch all PCS accessors again, now it is a good time to check
for error codes from the hardware access as well. We can't propagate the
errors very far due to phylink returning void for mac_config and
mac_link_up, but at least we print them to the console.

phylink doesn't have much option on what it could do if we error out at
those points - I suppose we could print a non-specific error and then
lock-out the interface in a similar way that phylib does, but to me that
seems really unfriendly if you're remotely accessing a box and the error
is intermittent.

I would like to have intermittent errors at this level logged, because
to me they would be quite unexpected and I would like to have some rope
to pull while debugging - an error code, something.

If there's an error of any sort, the interface won't be fully
initialized anyway, so not functional.

The reason why I added error checking in this patch is because I was
working on the MDIO bus accessors and I wanted to make sure that the
errors returned there are propagated somewhere.

Yes, makes sense there, but doesn't make sense if one is using the MMIO
accessors and have no errors to check...

My argument is - if you print an error at the lower levels, you can be
more specific about what failed. If you do it in phylink, you can only
say "oh, the blah_config() call failed" - which isn't particularly
useful.

Yes, we do this for some of the newly introduced methods, e.g. the
pcs_config() method - and there all we can say is:

                if (err < 0)
                        phylink_err(pl, "pcs_config failed: %pe\n",
                                    ERR_PTR(err));

So would you like me to make any change to the code, or is it just that
I mentioned in the commit message that the phylink methods return void?