Date:   Thu, 6 Jan 2022 11:56:56 +0800
From:   zhuyinbo <zhuyinbo@...ngson.cn>
To:     andrew@...n.ch, hkallweit1@...il.com, kuba@...nel.org,
        masahiroy@...nel.org, michal.lkml@...kovi.net,
        ndesaulniers@...gle.com
Cc:     davem@...emloft.net, kuba@...nel.org, masahiroy@...nel.org,
        michal.lkml@...kovi.net, ndesaulniers@...gle.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kbuild@...r.kernel.org, zhuyinbo@...ngson.cn,
        hkallweit1@...il.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org
Subject: Re: [PATCH v2 1/2] modpost: file2alias: fixup mdio alias garbled code
 in modules.alias



在 2022/1/5 上午11:33, zhuyinbo 写道:
> 
> 
> 在 2022/1/4 下午9:11, zhuyinbo 写道:
>>
>>
>> 在 2021/12/7 下午5:41, zhuyinbo 写道:
>>>
>>>
>>> 在 2021/12/1 上午8:38, Andrew Lunn 写道:
>>>>> However, this won't work for PHY devices created _before_ the kernel
>>>>> has mounted the rootfs, whether or not they end up being used. So,
>>>>> every PHY mentioned in DT will be created before the rootfs is 
>>>>> mounted,
>>>>> and none of these PHYs will have their modules loaded.
>>>>
>>>> Hi Russell
>>>>
>>>> I think what you are saying here is, if the MAC or MDIO bus driver is
>>>> built in, the PHY driver also needs to be built in?
>>>>
>>>> If the MAC or MDIO bus driver is a module, it means the rootfs has
>>>> already been mounted in order to get these modules. And so the PHY
>>>> driver as a module will also work.
>>>>
>>>>> I believe this is the root cause of Yinbo Zhu's issue.
>>>
>>> I think you should be right and I had did lots of test but use 
>>> rquest_module it doesn't load marvell module, and dts does't include 
>>> any phy node. even though I was use "marvell" as input's args of 
>>> request_module.
>>>>
>>>> You are speculating that in Yinbo Zhu case, the MAC driver is built
>>>> in, the PHY is a module. The initial request for the firmware fails.
>>>> Yinbo Zhu would like udev to try again later when the modules are
>>>> available.
>>>>
>>>>> What we _could_ do is review all device trees and PHY drivers to see
>>>>> whether DT modaliases are ever used for module loading. If they 
>>>>> aren't,
>>>>> then we _could_ make the modalias published by the kernel conditional
>>>>> on the type of mdio device - continue with the DT approach for non-PHY
>>>>> devices, and switch to the mdio: scheme for PHY devices. I repeat, 
>>>>> this
>>>>> can only happen if no PHY drivers match using the DT scheme, otherwise
>>>>> making this change _will_ cause a regression.
>>>>
>>>
>>>> Take a look at
>>>> drivers/net/mdio/of_mdio.c:whitelist_phys[] and the comment above it.
>>>>
>>>> So there are some DT blobs out there with compatible strings for
>>>> PHYs. I've no idea if they actually load that way, or the standard PHY
>>>> mechanism is used.
>>>>
>>>>     Andrew
>>>>
>>>
>>>
>>>  > That is not true universally for all MDIO though - as
>>>  > xilinx_gmii2rgmii.c clearly shows. That is a MDIO driver which 
>>> uses DT
>>>  > the compatible string to do the module load. So, we have proof there
>>>  > that Yinbo Zhu's change will definitely cause a regression which we
>>>  > can not allow.
>>>
>>> I don't understand that what you said about regression.  My patch 
>>> doesn't cause  xilinx_gmii2rgmii.c driver load fail, in this time 
>>> that do_of_table and platform_uevent will be responsible "of" type 
>>> driver auto load and my patch was responsible for "mdio" type driver 
>>> auto load,
>>> In default code. There are request_module to load phy driver, but as 
>>> Russell King said that request_module doesn't garantee auto load will 
>>> always work well, but udev mechanism can garantee it. and udev 
>>> mechaism is more mainstream, otherwise mdio_uevent is useless. if use 
>>> udev mechanism that my patch was needed. and if apply my patch it 
>>> doesn't cause request_module mechaism work bad because I will add 
>>> following change:
>>>
>>>
>>>
>>> -       ret = request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT,
>>> -                            MDIO_ID_ARGS(phy_id));
>>> +       ret = request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT, phy_id);
>>>          /* We only check for failures in executing the usermode binary,
>>>           * not whether a PHY driver module exists for the PHY ID.
>>>           * Accept -ENOENT because this may occur in case no 
>>> initramfs exists,
>>> diff --git a/include/linux/mod_devicetable.h 
>>> b/include/linux/mod_devicetable.h
>>> index 7bd23bf..bc6ea0d 100644
>>> --- a/include/linux/mod_devicetable.h
>>> +++ b/include/linux/mod_devicetable.h
>>> @@ -600,16 +600,7 @@ struct platform_device_id {
>>>   #define MDIO_NAME_SIZE         32
>>>   #define MDIO_MODULE_PREFIX     "mdio:"
>>>
>>> -#define MDIO_ID_FMT 
>>> "%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u"
>>> -#define MDIO_ID_ARGS(_id) \
>>> -       ((_id)>>31) & 1, ((_id)>>30) & 1, ((_id)>>29) & 1, 
>>> ((_id)>>28) & 1, \
>>> -       ((_id)>>27) & 1, ((_id)>>26) & 1, ((_id)>>25) & 1, 
>>> ((_id)>>24) & 1, \
>>> -       ((_id)>>23) & 1, ((_id)>>22) & 1, ((_id)>>21) & 1, 
>>> ((_id)>>20) & 1, \
>>> -       ((_id)>>19) & 1, ((_id)>>18) & 1, ((_id)>>17) & 1, 
>>> ((_id)>>16) & 1, \
>>> -       ((_id)>>15) & 1, ((_id)>>14) & 1, ((_id)>>13) & 1, 
>>> ((_id)>>12) & 1, \
>>> -       ((_id)>>11) & 1, ((_id)>>10) & 1, ((_id)>>9) & 1, ((_id)>>8) 
>>> & 1, \
>>> -       ((_id)>>7) & 1, ((_id)>>6) & 1, ((_id)>>5) & 1, ((_id)>>4) & 
>>> 1, \
>>> -       ((_id)>>3) & 1, ((_id)>>2) & 1, ((_id)>>1) & 1, (_id) & 1
>>> +#define MDIO_ID_FMT "p%08x"
>>>
>>>
>>>
>>
>>  > > > > However, this won't work for PHY devices created _before_ the 
>> kernel
>>  > > > > has mounted the rootfs, whether or not they end up being 
>> used. So,
>>  > > > > every PHY mentioned in DT will be created before the rootfs 
>> is mounted,
>>  > > > > and none of these PHYs will have their modules loaded.
>>  > > >
>>  > > > Hi Russell
>>  > > >
>>  > > > I think what you are saying here is, if the MAC or MDIO bus 
>> driver is
>>  > > > built in, the PHY driver also needs to be built in?
>>  > > >
>>  > > > If the MAC or MDIO bus driver is a module, it means the rootfs has
>>  > > > already been mounted in order to get these modules. And so the PHY
>>  > > > driver as a module will also work.
>>  > > >
>>  > > > > I believe this is the root cause of Yinbo Zhu's issue.
>>  > >
>>  > > I think you should be right and I had did lots of test but use 
>> rquest_module
>>  > > it doesn't load marvell module, and dts does't include any phy 
>> node. even
>>  > > though I was use "marvell" as input's args of request_module.
>>
>>  > Please can you report the contents of /proc/sys/kernel/modprobe, and
>>  > the kernel configuration of CONFIG_MODPROBE_PATH. I wonder if your
>>  > userspace has that module loading mechanism disabled, or your kernel
>>  > has CONFIG_MODPROBE_PATH as an empty string.
>>
>>  > If the module is not present by the time this call is made, then
>>  > even if you load the appropriate driver module later, that module
>>  > will not be used - the PHY will end up being driven by the generic
>>  > clause 22 driver.
>>
>>  > > > That is not true universally for all MDIO though - as
>>  > > > xilinx_gmii2rgmii.c clearly shows. That is a MDIO driver which 
>> uses DT
>>  > > > the compatible string to do the module load. So, we have proof 
>> there
>>  > > > that Yinbo Zhu's change will definitely cause a regression 
>> which we
>>  > > > can not allow.
>>  > >
>>  > > I don't understand that what you said about regression.  My patch 
>> doesn't
>>  > > cause  xilinx_gmii2rgmii.c driver load fail, in this time that 
>> do_of_table
>>  > >and platform_uevent will be responsible "of" type driver auto load 
>> and my
>>  > > patch was responsible for "mdio" type driver auto load,
>>
>>  > xilinx_gmii2rgmii is not a platform driver. It is a mdio driver:
>>
>>  > static struct mdio_driver xgmiitorgmii_driver = {
>>                ^^^^^^^^^^^
>>
>>  > Therefore, platform_uevent() is irrelevant since this will never match
>>  > a platform device. It will only match mdio devices, and the uevent
>>  > generation for that is via mdio_uevent() which is the function you
>>  > are changing.
>>
>>
>> static const struct of_device_id xgmiitorgmii_of_match[] = {
>>          { .compatible = "xlnx,gmii-to-rgmii-1.0" },
>>          {},
>> };
>> MODULE_DEVICE_TABLE(of, xgmiitorgmii_of_match);
>>
>> static struct mdio_driver xgmiitorgmii_driver = {
>>          .probe  = xgmiitorgmii_probe,
>>          .mdiodrv.driver = {
>>                  .name = "xgmiitorgmii",
>>                  .of_match_table = xgmiitorgmii_of_match,
>>          },
>> };
>>  From the present point of view, no matter what the situation, my 
>> supplement can cover udev or request_module for auto load module.
>>
>> if that phy driver isn't platform driver my patch cover it I think 
>> there is no doubt, if phy driver is platform driver and platform 
>> driver udev will cover it. My only requestion is the request_module 
>> not work well.
>>
>> about xgmiitorgmii_of_match that it belongs to platform driver load, 
>> please you note. and about your doubt usepace whether disable module 
>> load that module load function is okay becuase other device driver 
>> auto load is okay.
>>
>>  > > In default code. There are request_module to load phy driver, but 
>> as > Russell
>>  > > King said that request_module doesn't garantee auto load will 
>> always work
>>  > > well, but udev mechanism can garantee it. and udev mechaism is more
>>  > > mainstream, otherwise mdio_uevent is useless. if use udev 
>> mechanism that my
>>  > > patch was needed. and if apply my patch it doesn't cause 
>> request_module
>>  > > mechaism work bad because I will add following change:
>>
>>  > Please report back what the following command produces on your
>>  > problem system:
>>
>>  > /sbin/modprobe -vn mdio:00000001010000010000110111010001
>>
>>  > Thanks.
>>
>> [root@...alhost ~]# lsmod | grep marvell
>> [root@...alhost ~]# ls 
>> /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>> /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>> [root@...alhost ~]# /sbin/modprobe -vn 
>> mdio:00000001010000010000110111010001
>> insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>> insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>> [root@...alhost ~]#
>> [root@...alhost ~]# cat /proc/sys/kernel/modprobe
>> /sbin/modprobe
>>
>> BRs,
>> Yinbo
> 
>  > On Tue, Jan 04, 2022 at 09:11:56PM +0800, zhuyinbo wrote:
>  > > From the present point of view, no matter what the situation, my 
> supplement
>  > > can cover udev or request_module for auto load module.
>  > >
>  > > if that phy driver isn't platform driver my patch cover it I think 
> there is
>  > > no doubt, if phy driver is platform driver and platform driver udev 
> will
>  > > cover it. My only requestion is the request_module not work well.
>  > >
>  > > about xgmiitorgmii_of_match that it belongs to platform driver 
> load, please
>  > > you note. and about your doubt usepace whether disable module load 
> that
>  > > module load function is okay becuase other device driver auto load 
> is okay.
> 
>  > xgmiitorgmii is *not* a platform driver.
> 
> For the module loading function, you need to focus on the first args 
> "of" in function MODULE_ DEVICE_TABLE, not the definition type of this 
> driver.  for "of" type that must platform covert it !
> 
>  > > > Please report back what the following command produces on your
>  > > > problem system:
>  > > >
>  > > > /sbin/modprobe -vn mdio:00000001010000010000110111010001
>  > > >
>  > > > Thanks.
>  > >
>  > > [root@...alhost ~]# lsmod | grep marvell
>  > > [root@...alhost ~]# ls
>  > > /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>  > > /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>  > > [root@...alhost ~]# /sbin/modprobe -vn 
>  >mdio:00000001010000010000110111010001
>  > > insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>  > > insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>  > > [root@...alhost ~]#
>  > > [root@...alhost ~]# cat /proc/sys/kernel/modprobe
>  > > /sbin/modprobe
> 
>  > Great, so the current scheme using "mdio:<binary digits>" works
>  > perfectly for you. What is missing is having that modalias in the
>  > uevent file.
> No, "lsmod | grep marvel" is NULL, so "mdio:<binary digits>" doesn't 
> work well. and that lost information is string that match do_mdio_entry 
> Hexadecimal string or binary digits and I add my patch use hexadecimal 
> and change do_mdio_entry to fix issue was to consider that different 
> "Revision Number" represent that phy hardware may be has some 
> difference, so I think we should not blindly load the corresponding PHY 
> driver. It is appropriate to match the PHY ID exactly. for 
> example,following code is marvell phy for 88e1510, which include it's 
> initial function for phy. and my platform hardware use 88e1512, if 
> doesn't change do_mdio_entry code, use "?" to match it, which represent
> 88e1512 hardware to match 881510 driver. of course if 88e1510 driver can 
> compatible with 88e1512 phy, it is okay, but for all kinds of ethernet 
> phy hc and hcd you can ensure it always has a good compatible for that?
> Like 88e1510 driver, it is compatible with 88e1512 and has no problem. 
> In fact, I'm not sure if there is a problem with the 88e1512 loaded 
> 88e1510 driver. So I modified do_mdio_entry is used for full matching, 
> and it can cover above all issue.
> 
>                  .phy_id = MARVELL_PHY_ID_88E1510,
>                  .phy_id_mask = MARVELL_PHY_ID_MASK,
>                  .name = "Marvell 88E1510",
>                  .features = PHY_GBIT_FEATURES | SUPPORTED_FIBRE,
>                  .flags = PHY_HAS_INTERRUPT,
>                  .probe = &m88e1510_probe,
>                  .config_init = &m88e1510_config_init,
>                  .config_aneg = &m88e1510_config_aneg,
>                  .read_status = &marvell_read_status,
> 
> 
>  > So, my patch on the 4th December should cause the marvell module to
>  > be loaded at boot time. Please test that patch ASAP, which I have
>  > already asked you to do. I'll include it again in this email so you
>  > don't have to hunt for it.
> 
>  > 8<===
>  > From: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
>  > Subject: [PATCH] net: phy: generate PHY mdio modalias
> 
>  > The modalias string provided in the uevent sysfs file does not conform
>  > to the format used in PHY driver modules. One of the reasons is that
>  > udev loading of PHY driver modules has not been an expected use case.
> 
>  > This patch changes the MODALIAS entry for only PHY devices from:
>  >         MODALIAS=of:Nethernet-phyT(null)
>  > to:
>  >         MODALIAS=mdio:00000000001000100001010100010011
> 
>  > Other MDIO devices (such as DSA) remain as before.
> 
>  > However, having udev automatically load the module has the advantage
>  > of making use of existing functionality to have the module loaded
>  > before the device is bound to the driver, thus taking advantage of
>  > multithreaded boot systems, potentially decreasing the boot time.
> 
>  > However, this patch will not solve any issues with the driver module
>  > not being loaded prior to the network device needing to use the PHY.
>  > This is something that is completely out of control of any patch to
>  > change the uevent mechanism.
> 
>  > Reported-by: Yinbo Zhu <zhuyinbo@...ngson.cn>
>  > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
>  > ---
>  > drivers/net/phy/mdio_bus.c   |  8 ++++++++
>  > drivers/net/phy/phy_device.c | 14 ++++++++++++++
>  > include/linux/mdio.h         |  2 ++
>  > 3 files changed, 24 insertions(+)
> 
>  > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>  > index 4638d7375943..663bd98760fb 100644
>  > --- a/drivers/net/phy/mdio_bus.c
>  > +++ b/drivers/net/phy/mdio_bus.c
>  > @@ -1010,8 +1010,16 @@ static int mdio_bus_match(struct device *dev, 
>  > > struct device_driver *drv)
> 
>  >  static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
>   > {
>  > +    struct mdio_device *mdio = to_mdio_device(dev);
>  >     int rc;
>  >
>  > +    /* Use the device-specific uevent if specified */
>  > +    if (mdio->bus_uevent) {
>  > +        rc = mdio->bus_uevent(mdio, env);
>  > +        if (rc != -ENODEV)
>  > +            return rc;
>  > +    }
>  > +
>  >     /* Some devices have extra OF data and an OF-style MODALIAS */
>  >      rc = of_device_uevent_modalias(dev, env);
>  >     if (rc != -ENODEV)
>  > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>  > index 23667658b9c6..f4c2057f0202 100644
>  > --- a/drivers/net/phy/phy_device.c
>  > +++ b/drivers/net/phy/phy_device.c
>  > @@ -563,6 +563,19 @@ static int phy_request_driver_module(struct 
> phy_device *dev, u32 phy_id)
>  >      return 0;
>  >  }
> 
>  > +static int phy_bus_uevent(struct mdio_device *mdiodev,
>  > +              struct kobj_uevent_env *env)
>  > +{
>  > +    struct phy_device *phydev;
>  > +
>  > +    phydev = container_of(mdiodev, struct phy_device, mdio);
>  > +
>  > +    add_uevent_var(env, "MODALIAS=" MDIO_MODULE_PREFIX MDIO_ID_FMT,
>  > +               MDIO_ID_ARGS(phydev->phy_id));
>  > +
>  > +    return 0;
>  > +}
>  > +
>  >  struct phy_device *phy_device_create(struct mii_bus *bus, int addr, 
> u32 phy_id,
>  >                       bool is_c45,
>  >                       struct phy_c45_device_ids *c45_ids)
>  > @@ -582,6 +595,7 @@ struct phy_device *phy_device_create(struct 
> mii_bus *bus, int addr, u32 phy_id,
>  >      mdiodev->dev.type = &mdio_bus_phy_type;
>  >      mdiodev->bus = bus;
>  >     mdiodev->bus_match = phy_bus_match;
>  > +    mdiodev->bus_uevent = phy_bus_uevent;
>  >     mdiodev->addr = addr;
>  >     mdiodev->flags = MDIO_DEVICE_FLAG_PHY;
>  >     mdiodev->device_free = phy_mdio_device_free;
>  > diff --git a/include/linux/mdio.h b/include/linux/mdio.h
>  > index df9c96e56907..5c6676d3de23 100644
>  > --- a/include/linux/mdio.h
>  > +++ b/include/linux/mdio.h
>  > @@ -38,6 +38,8 @@ struct mdio_device {
>  >      char modalias[MDIO_NAME_SIZE];
> 
>  >      int (*bus_match)(struct device *dev, struct device_driver *drv);
>  > +    int (*bus_uevent)(struct mdio_device *mdiodev,
>  > +              struct kobj_uevent_env *env);
>  >     void (*device_free)(struct mdio_device *mdiodev);
>  >     void (*device_remove)(struct mdio_device *mdiodev);
> your patch I have a try and it can make marvel driver auto-load. 
> However, you need to evaluate the above compatibility issues !
> in addition, if phy id register work bad or other case, you dont' read 
> phy id from phy.  your patch will not work well. so you shoud definition 
> a any_phy_id, of course, The most critical issue is the above driver 
> compatibility, please you note.
> 
> in additon, I have never received your email before. I have to check 
> patchwork every time, so if you have a advice that could you send a mail 
> to zhuyinbo@...ngson.cn .
> 
> Thanks,
> 
> BRs,
> Yinbo Zhu.
Hi phy maintainer,

What's your viewpoint?

Thanks
BRs
Yinbo Zhu.

Powered by blists - more mailing lists