Re: [RFC PATCH v1 0/4] Rust abstractions for network PHY drivers

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

Hi,

On Mon, 18 Sep 2023 19:46:57 -0400
Trevor Gross <tmgross@umich.edu> wrote:

>> > [1]: @Fujita I know you have mentioned there are times where a driver
>> > does not always have one mask. I can't find anything and am not sure
>> > how this would work since `phy_driver` has `phy_id` and `phy_id_mask`
>> > fields - do you mind linking an example? The above could probably be
>> > adjusted to work with this
>>
>> drivers/met/phy/meson-gxl.c does some funky things:
>>
>> static struct phy_driver meson_gxl_phy[] = {
>>         {
>>                 PHY_ID_MATCH_EXACT(0x01814400),
>>                 .name           = "Meson GXL Internal PHY",
>> [ ... ]
>>         }, {
>>                 PHY_ID_MATCH_EXACT(0x01803301),
>>                 .name           = "Meson G12A Internal PHY",
>> [ ... ]
>>         },
>> };
>>
>> static struct mdio_device_id __maybe_unused meson_gxl_tbl[] = {
>>         { PHY_ID_MATCH_VENDOR(0x01814400) },
>>         { PHY_ID_MATCH_VENDOR(0x01803301) },
>>         { }
>>
>> So the phy_driver wants an exact match, but the module is loaded based
>> on the vendor OUIs.
>>
>> icplus.c:
>>
>> static struct mdio_device_id __maybe_unused icplus_tbl[] = {
>>         { PHY_ID_MATCH_MODEL(IP175C_PHY_ID) },
>>         { PHY_ID_MATCH_MODEL(IP1001_PHY_ID) },
>>         { PHY_ID_MATCH_EXACT(IP101A_PHY_ID) },
>>         { }
>> };
>>
>> The devices seem to not have unique IDs, so:
>>
>> static int ip101a_g_match_phy_device(struct phy_device *phydev, bool ip101a)
>> {
>> [ ... ]
>> }
>>
>> Its a variant on Murphy's law. If vendors can get something wrong,
>> some vendor will get it wrong at some point.
>>
>>      Andrew
> 
> Thanks for the references here, I see I misunderstood exactly the
> problem was. In this case, maybe we could do something like this:
> 
>     trait Driver {
>         const PHY_ID: u32,
>         const PHY_ID_MASK: PhyIdMask,
>         // Default to the same value
>         const PHY_DEVICE_ID_MASK: PhyIdMask = Self::PHY_ID_MASK,
>         const NAME: &'static CStr;
>         // ...
>     }
> 
> `PHY_ID_MASK` would be used to create the `phy_driver`,
> `PHY_DEVICE_ID_MASK`  creates the `mdio_device`. That can be
> overridden in cases like the Meson G where they aren't the same.
> 
> I think that the icplus situation is already possible with `match_phy_device`

drivers/net/phy/adin1100.c

static struct phy_driver adin_driver[] = {
	{
		.phy_id			= PHY_ID_ADIN1100,
		.phy_id_mask		= 0xffffffcf,
		.name			= "ADIN1100",
		.get_features		= adin_get_features,
		.soft_reset		= adin_soft_reset,
		.probe			= adin_probe,
		.config_aneg		= adin_config_aneg,
		.read_status		= adin_read_status,
		.set_loopback		= adin_set_loopback,
		.suspend		= adin_suspend,
		.resume			= adin_resume,
		.get_sqi		= adin_get_sqi,
		.get_sqi_max		= adin_get_sqi_max,
	},
};

module_phy_driver(adin_driver);

static struct mdio_device_id __maybe_unused adin_tbl[] = {
	{ PHY_ID_MATCH_MODEL(PHY_ID_ADIN1100) },
	{ PHY_ID_MATCH_MODEL(PHY_ID_ADIN1110) },
	{ PHY_ID_MATCH_MODEL(PHY_ID_ADIN2111) },
	{ }
};

This registers one driver but registers three mdio_device_ids.

bcm87xx.c registers two drivers but registers no
mdio_device_id. realtek.c registers 10 drivers but only one
mdio_device_id. There are some drivers that do unique.

I think that to separate registations of drivers and mdio_device_id is
the cleanest way.