RE: [PATCH net-next v10 06/11] net: ethernet: oa_tc6: Support for vendor specific MMS

[Selvamani Rajagopal]

> -----Original Message-----
> From: netdev-bot+sashiko@xxxxxxxxxx <netdev-bot+sashiko@xxxxxxxxxx>
> To: Selvamani Rajagopal <Selvamani.Rajagopal@xxxxxxxxxx>
> Subject: Re: [PATCH net-next v10 06/11] net: ethernet: oa_tc6: Support for vendor
> specific MMS
> 
> net: ethernet: oa_tc6: Support for vendor specific MMS
> 
> This adds oa_tc6_set_vend1_mms(), which lets a vendor driver map
> MDIO_MMD_VEND1 to one of the vendor specific MMS values (10 to 15)
> allowed by the OPEN Alliance TC6 specification. The C45 MMS lookup now
> takes the tc6 context. The new vend1_mms field defaults to -EOPNOTSUPP
> in oa_tc6_init().
 
> [Severity: Medium]
> 
> Can a failed capability read oops on a NULL phydev? In oa_tc6_init(),
> oa_tc6_update_standard_capability() is called before oa_tc6_phy_init(),
> and on failure it does goto phy_exit.


Yes. It can. Will fix it.

> 
> 
> [Severity: Medium]
> Does this setter run too late to affect PHY probing and
> initialization?
> 
> struct oa_tc6 is allocated inside oa_tc6_init(), and the caller only
> gets the pointer back once oa_tc6_init() returns. By then
> oa_tc6_init() has already run oa_tc6_phy_init(), 

True. At present, the place where vend1_mms set is not the right place.
MMS value could be sent through quirks structure that we have now.

Considering the fact that this API isn't used by the PHY driver now and 
MAC driver can access MMS12 directly through read/write API, I am
going to remove this API. When PHY driver needs it, probably when PHY 
driver is changed to work in interrupt mode, we can re-introduce this 
API the right way.

> 
> [Severity: High]
> This patch doesn't touch it, but there is a problem further down in
> oa_tc6_init(). The earlier patch in this series, "net: ethernet: oa_tc6:
> Support for hardware timestamp", added it, and it is still there at the
> tip of the series.
> 
> Can the capability read failure path oops with a NULL phydev? In
> oa_tc6_init():


Yes. This issue was overlooked when we moved the API up. Will fix it.

> 
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-s2500-
> mac-phy-support-v10-0-31b5a69b85b7%40onsemi.com
> <https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-s2500-mac-phy-support-v10-0-31b5a69b85b7%40onsemi.com
> netdev-ai.bots.linux.dev>

pw-bot: cr