| Message ID | 20210826074526.825517-3-saravanak@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix rtl8366rb issues with fw_devlink=on | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 13 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Thu, Aug 26, 2021 at 12:45:25AM -0700, Saravana Kannan wrote: > This is just a quick fix to make this driver work with fw_devlink=on. > The proper fix might need a significant amount of rework of the driver > of the framework to use a component device model. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/net/dsa/realtek-smi-core.c | 7 +++++++ > 1 file changed, 7 insertions(+) "quick" fixes are nice, but who is going to do the real work here to fix this properly if this series is accepted? thanks, greg k-h
On 8/26/2021 9:45 AM, Saravana Kannan wrote: > This is just a quick fix to make this driver work with fw_devlink=on. > The proper fix might need a significant amount of rework of the driver > of the framework to use a component device model. > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/net/dsa/realtek-smi-core.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/net/dsa/realtek-smi-core.c b/drivers/net/dsa/realtek-smi-core.c > index 8e49d4f85d48..f79c174f4954 100644 > --- a/drivers/net/dsa/realtek-smi-core.c > +++ b/drivers/net/dsa/realtek-smi-core.c > @@ -394,6 +394,13 @@ static int realtek_smi_probe(struct platform_device *pdev) > var = of_device_get_match_data(dev); > np = dev->of_node; > > + /* This driver assumes the child PHYs would be probed successfully > + * before this functions returns. That's not a valid assumption, but > + * let fw_devlink know so that this driver continues to function with > + * fw_devlink=on. > + */ > + np->fwnode.flags |= FWNODE_FLAG_BROKEN_PARENT; Are we positive this is not an issue that applies generally to all DSA drivers?
Hi Saravana, From looking at the code, the Marvell DSA driver mv88e6xxx may also suffer from the same problem if fw_devlink=on. Maybe somebody (Andrew?) could test so that you know whether include a simlar patch to that driver in your series. Other drivers may be effected too - as Andrew said in the other thread, this is not an uncommon pattern for DSA drivers. On 8/26/21 9:45 AM, Saravana Kannan wrote: > This is just a quick fix to make this driver work with fw_devlink=on. > The proper fix might need a significant amount of rework of the driver > of the framework to use a component device model. > > Signed-off-by: Saravana Kannan <saravanak@google.com> With the caveat that it's a test with my RFC rtl8365mb subdriver... Tested-by: Alvin Šipraga <alsi@bang-olufsen.dk> Kind regards, Alvin > --- > drivers/net/dsa/realtek-smi-core.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/net/dsa/realtek-smi-core.c b/drivers/net/dsa/realtek-smi-core.c > index 8e49d4f85d48..f79c174f4954 100644 > --- a/drivers/net/dsa/realtek-smi-core.c > +++ b/drivers/net/dsa/realtek-smi-core.c > @@ -394,6 +394,13 @@ static int realtek_smi_probe(struct platform_device *pdev) > var = of_device_get_match_data(dev); > np = dev->of_node; > > + /* This driver assumes the child PHYs would be probed successfully > + * before this functions returns. That's not a valid assumption, but > + * let fw_devlink know so that this driver continues to function with > + * fw_devlink=on. > + */ > + np->fwnode.flags |= FWNODE_FLAG_BROKEN_PARENT; > + > smi = devm_kzalloc(dev, sizeof(*smi) + var->chip_data_sz, GFP_KERNEL); > if (!smi) > return -ENOMEM; >
On Thu, Aug 26, 2021 at 4:29 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote: > > Hi Saravana, > > From looking at the code, the Marvell DSA driver mv88e6xxx may also > suffer from the same problem if fw_devlink=on. Maybe somebody (Andrew?) > could test so that you know whether include a simlar patch to that > driver in your series. > > Other drivers may be effected too - as Andrew said in the other thread, > this is not an uncommon pattern for DSA drivers. > > On 8/26/21 9:45 AM, Saravana Kannan wrote: > > This is just a quick fix to make this driver work with fw_devlink=on. > > The proper fix might need a significant amount of rework of the driver > > of the framework to use a component device model. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > With the caveat that it's a test with my RFC rtl8365mb subdriver... > > Tested-by: Alvin Šipraga <alsi@bang-olufsen.dk> Thanks for testing. And just to be sure we are all on the same page: Without this patch the PHYs get handled by the Generic PHY driver. With this patch, the PHYs are handled by their specific driver. Correct? -Saravana > > Kind regards, > Alvin > > > --- > > drivers/net/dsa/realtek-smi-core.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/net/dsa/realtek-smi-core.c b/drivers/net/dsa/realtek-smi-core.c > > index 8e49d4f85d48..f79c174f4954 100644 > > --- a/drivers/net/dsa/realtek-smi-core.c > > +++ b/drivers/net/dsa/realtek-smi-core.c > > @@ -394,6 +394,13 @@ static int realtek_smi_probe(struct platform_device *pdev) > > var = of_device_get_match_data(dev); > > np = dev->of_node; > > > > + /* This driver assumes the child PHYs would be probed successfully > > + * before this functions returns. That's not a valid assumption, but > > + * let fw_devlink know so that this driver continues to function with > > + * fw_devlink=on. > > + */ > > + np->fwnode.flags |= FWNODE_FLAG_BROKEN_PARENT; > > + > > smi = devm_kzalloc(dev, sizeof(*smi) + var->chip_data_sz, GFP_KERNEL); > > if (!smi) > > return -ENOMEM; > >
On Thu, Aug 26, 2021 at 4:25 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 8/26/2021 9:45 AM, Saravana Kannan wrote: > > This is just a quick fix to make this driver work with fw_devlink=on. > > The proper fix might need a significant amount of rework of the driver > > of the framework to use a component device model. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > --- > > drivers/net/dsa/realtek-smi-core.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/net/dsa/realtek-smi-core.c b/drivers/net/dsa/realtek-smi-core.c > > index 8e49d4f85d48..f79c174f4954 100644 > > --- a/drivers/net/dsa/realtek-smi-core.c > > +++ b/drivers/net/dsa/realtek-smi-core.c > > @@ -394,6 +394,13 @@ static int realtek_smi_probe(struct platform_device *pdev) > > var = of_device_get_match_data(dev); > > np = dev->of_node; > > > > + /* This driver assumes the child PHYs would be probed successfully > > + * before this functions returns. That's not a valid assumption, but > > + * let fw_devlink know so that this driver continues to function with > > + * fw_devlink=on. > > + */ > > + np->fwnode.flags |= FWNODE_FLAG_BROKEN_PARENT; > > Are we positive this is not an issue that applies generally to all DSA > drivers? Possibly, but I don't know enough about that framework. Let me respond to the email from Andrew[1] and give a summary there. We can go from there. [1] - https://lore.kernel.org/lkml/YSeTdb6DbHbBYabN@lunn.ch/#t -Saravana
On 8/26/21 7:26 PM, Saravana Kannan wrote: > On Thu, Aug 26, 2021 at 4:29 AM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote: >> >> Hi Saravana, >> >> From looking at the code, the Marvell DSA driver mv88e6xxx may also >> suffer from the same problem if fw_devlink=on. Maybe somebody (Andrew?) >> could test so that you know whether include a simlar patch to that >> driver in your series. >> >> Other drivers may be effected too - as Andrew said in the other thread, >> this is not an uncommon pattern for DSA drivers. >> >> On 8/26/21 9:45 AM, Saravana Kannan wrote: >>> This is just a quick fix to make this driver work with fw_devlink=on. >>> The proper fix might need a significant amount of rework of the driver >>> of the framework to use a component device model. >>> >>> Signed-off-by: Saravana Kannan <saravanak@google.com> >> >> With the caveat that it's a test with my RFC rtl8365mb subdriver... >> >> Tested-by: Alvin Šipraga <alsi@bang-olufsen.dk> > > Thanks for testing. And just to be sure we are all on the same page: > Without this patch the PHYs get handled by the Generic PHY driver. > With this patch, the PHYs are handled by their specific driver. > Correct? Yes, both statements are correct. Alvin > > -Saravana > >> >> Kind regards, >> Alvin >> >>> --- >>> drivers/net/dsa/realtek-smi-core.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/net/dsa/realtek-smi-core.c b/drivers/net/dsa/realtek-smi-core.c >>> index 8e49d4f85d48..f79c174f4954 100644 >>> --- a/drivers/net/dsa/realtek-smi-core.c >>> +++ b/drivers/net/dsa/realtek-smi-core.c >>> @@ -394,6 +394,13 @@ static int realtek_smi_probe(struct platform_device *pdev) >>> var = of_device_get_match_data(dev); >>> np = dev->of_node; >>> >>> + /* This driver assumes the child PHYs would be probed successfully >>> + * before this functions returns. That's not a valid assumption, but >>> + * let fw_devlink know so that this driver continues to function with >>> + * fw_devlink=on. >>> + */ >>> + np->fwnode.flags |= FWNODE_FLAG_BROKEN_PARENT; >>> + >>> smi = devm_kzalloc(dev, sizeof(*smi) + var->chip_data_sz, GFP_KERNEL); >>> if (!smi) >>> return -ENOMEM; >>>
diff --git a/drivers/net/dsa/realtek-smi-core.c b/drivers/net/dsa/realtek-smi-core.c index 8e49d4f85d48..f79c174f4954 100644 --- a/drivers/net/dsa/realtek-smi-core.c +++ b/drivers/net/dsa/realtek-smi-core.c @@ -394,6 +394,13 @@ static int realtek_smi_probe(struct platform_device *pdev) var = of_device_get_match_data(dev); np = dev->of_node; + /* This driver assumes the child PHYs would be probed successfully + * before this functions returns. That's not a valid assumption, but + * let fw_devlink know so that this driver continues to function with + * fw_devlink=on. + */ + np->fwnode.flags |= FWNODE_FLAG_BROKEN_PARENT; + smi = devm_kzalloc(dev, sizeof(*smi) + var->chip_data_sz, GFP_KERNEL); if (!smi) return -ENOMEM;
This is just a quick fix to make this driver work with fw_devlink=on. The proper fix might need a significant amount of rework of the driver of the framework to use a component device model. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/net/dsa/realtek-smi-core.c | 7 +++++++ 1 file changed, 7 insertions(+)