Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 16 Jun 2023 16:16:49 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     Jianhui Zhao <zhaojh329@gmail.com>, hkallweit1@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH V3] net: phy: Add sysfs attribute for PHY c45 identifiers.
Message-ID: <037cfb16-0d40-47bc-bafe-7a1b3964d6ec@lunn.ch>
References: <20230614134522.11169-1-zhaojh329@gmail.com>
 <ZIwUFXOCCKZYSUBi@shell.armlinux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZIwUFXOCCKZYSUBi@shell.armlinux.org.uk>
Precedence: bulk

On Fri, Jun 16, 2023 at 08:49:41AM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 14, 2023 at 09:45:22PM +0800, Jianhui Zhao wrote:
> > +static const struct attribute_group phy_dev_c45_ids_group = {
> > +	.name = "c45_ids",
> > +	.attrs = phy_c45_id_attrs
> > +};
> 
> One last thing - is there any point to creating these attributes if
> the PHY isn't c45?
> 
> We could add here:
> 
> 	.is_visible = phy_dev_c45_visible,
> 
> with:
> 
> static umode_t phy_dev_c45_visible(struct kobject *kobj,
> 				   struct attribute *attr, int foo)
> {
> 	struct phy_device *phydev = to_phy_device(kobj_to_dev(kobj));
> 
> 	return phydev->is_c45 ? attr->mode : 0;
> }
> 
> which will only show the c45 attributes if the PHY is a c45 PHY.
> 
> Andrew, any opinions?

There are PHYs which get detected via their C22 ID, but the driver
then uses C45. So phydev->is_c45 could be false yet the device does
have C45 IDs. But i don't see a good solution to this. If the point of
these values is to aid debugging matching devices to drivers, this
does not really matter. Its C22 ID is what will be used, and that
sysfs file will be populated.

So this .is_visible seems reasonable.

I suppose there is the flip condition. Do we want the C22 sysfs file
visible if there is no C22 ID? But that is probably ABI, we cannot
change it now.

   Andrew