Messages in this thread Patch in this message | | | Date | Mon, 12 Dec 2022 16:22:51 +0100 | From | Andrew Lunn <> | Subject | Re: [PATCH v6 net-next 3/5] drivers/net/phy: add connection between ethtool and phylib for PLCA |
| |
On Sun, Dec 11, 2022 at 08:34:14PM +0000, Russell King (Oracle) wrote: > On Sun, Dec 11, 2022 at 08:03:15PM +0100, Piergiorgio Beruto wrote: > > On Sun, Dec 11, 2022 at 12:23:53PM +0000, Russell King (Oracle) wrote: > > > On Sat, Dec 10, 2022 at 11:46:39PM +0100, Piergiorgio Beruto wrote: > > > > This patch adds the required connection between netlink ethtool and > > > > phylib to resolve PLCA get/set config and get status messages. > > > > > > > > Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com> > > > > --- > > > > drivers/net/phy/phy.c | 175 +++++++++++++++++++++++++++++++++++ > > > > drivers/net/phy/phy_device.c | 3 + > > > > include/linux/phy.h | 7 ++ > > > > 3 files changed, 185 insertions(+) > > > > > > > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > > > > index e5b6cb1a77f9..40d90ed2f0fb 100644 > > > > --- a/drivers/net/phy/phy.c > > > > +++ b/drivers/net/phy/phy.c > > > > @@ -543,6 +543,181 @@ int phy_ethtool_get_stats(struct phy_device *phydev, > > > > } > > > > EXPORT_SYMBOL(phy_ethtool_get_stats); > > > > > > > > +/** > > > > + * phy_ethtool_get_plca_cfg - Get PLCA RS configuration > > > > + * > > > > > > You shouldn't have an empty line in the comment here > > I was trying to follow the style of this file. All other functions start > > like this, including an empty line. Do you want me to: > > a) follow your indication and leave all other functions as they are? > > b) Change all functions docs to follow your suggestion? > > c) leave it as-is? > > > > Please, advise. > > Please see Documentation/doc-guide/kernel-doc.rst > > "Function parameters > ~~~~~~~~~~~~~~~~~~~ > > Each function argument should be described in order, immediately following > the short function description. Do not leave a blank line between the > function description and the arguments, nor between the arguments." > > Note the last sentence - there should _not_ be a blank line, so please > follow this for new submissions. I don't think we care enough to fix > what's already there though. > > > > > > > > > > + * @phydev: the phy_device struct > > > > + * @plca_cfg: where to store the retrieved configuration > > > > > > Maybe have an empty line, followed by a bit of text describing what this > > > function does and the return codes it generates? > > Again, I was trying to follow the style of the docs in this file. > > Do you still want me to add a description here? > > Convention is a blank line - as illustrated by the general format > in the documentation file I refer to above. > > > > > > > > > > + */ > > > > +int phy_ethtool_get_plca_cfg(struct phy_device *phydev, > > > > + struct phy_plca_cfg *plca_cfg) > > > > +{ > > > > + int ret; > > > > + > > > > + if (!phydev->drv) { > > > > + ret = -EIO; > > > > + goto out; > > > > + } > > > > + > > > > + if (!phydev->drv->get_plca_cfg) { > > > > + ret = -EOPNOTSUPP; > > Once more, all other functions in this file take the mutex -after- > > checking for phydev->drv and checking the specific function. Therefore, > > I assumed that was a safe thing to do. If not, should we fix all of > > these functions in this file? > > This is a review comment I've made already, but you seem to have ignored > it. Please ensure that new contributions are safe. Yes, existing code > may not be, and that's something we should fix, but your contribution > should at least be safer than the existing code.
I have a patch ready for fixing the cable test examples of performing the test before taking the lock. I was going to post it to net-next in a couple of weeks time. Or do you think it should be to net?
Andrew
| |