| Message ID | 20210419042722.27554-2-alice.guo@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | support soc_device_match to return -EPROBE_DEFER | expand |
First comment overall for the whole serie: Since it is the solution I had suggested when I reported the problem[1] I have no qualm on the approach, comments for individual patches follow. [1] http://lore.kernel.org/r/YGGZJjAxA1IO+/VU@atmark-techno.com Alice Guo (OSS) wrote on Mon, Apr 19, 2021 at 12:27:20PM +0800: > From: Alice Guo <alice.guo@nxp.com> > > In i.MX8M boards, the registration of SoC device is later than caam > driver which needs it. Caam driver needs soc_device_match to provide > -EPROBE_DEFER when no SoC device is registered and no > early_soc_dev_attr. This patch should be last in the set: you can't have soc_device_match return an error before its callers handle it. > Signed-off-by: Alice Guo <alice.guo@nxp.com> As the one who reported the problem I would have been appreciated being at least added to Ccs... I only happened to notice you posted this by chance. There is also not a single Fixes tag -- I believe this commit should have Fixes: 7d981405d0fd ("soc: imx8m: change to use platform driver") but I'm not sure how such tags should be handled in case of multiple patches fixing something.
Hi Alice, CC Arnd (soc_device_match() author) On Mon, Apr 19, 2021 at 6:28 AM Alice Guo (OSS) <alice.guo@oss.nxp.com> wrote: > From: Alice Guo <alice.guo@nxp.com> > > In i.MX8M boards, the registration of SoC device is later than caam > driver which needs it. Caam driver needs soc_device_match to provide > -EPROBE_DEFER when no SoC device is registered and no > early_soc_dev_attr. I'm wondering if this is really a good idea: soc_device_match() is a last-resort low-level check, and IMHO should be made available early on, so there is no need for -EPROBE_DEFER. > > Signed-off-by: Alice Guo <alice.guo@nxp.com> Thanks for your patch! > --- a/drivers/base/soc.c > +++ b/drivers/base/soc.c > @@ -110,6 +110,7 @@ static void soc_release(struct device *dev) > } > > static struct soc_device_attribute *early_soc_dev_attr; > +static bool soc_dev_attr_init_done = false; Do you need this variable? > > struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr) > { > @@ -157,6 +158,7 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr > return ERR_PTR(ret); > } > > + soc_dev_attr_init_done = true; > return soc_dev; > > out3: > @@ -246,6 +248,9 @@ const struct soc_device_attribute *soc_device_match( > if (!matches) > return NULL; > > + if (!soc_dev_attr_init_done && !early_soc_dev_attr) if (!soc_bus_type.p && !early_soc_dev_attr) > + return ERR_PTR(-EPROBE_DEFER); > + > while (!ret) { > if (!(matches->machine || matches->family || > matches->revision || matches->soc_id)) Gr{oetje,eeting}s, Geert
On Mon, Apr 19, 2021 at 10:20:13AM +0200, Geert Uytterhoeven wrote: > Hi Alice, > > CC Arnd (soc_device_match() author) > > On Mon, Apr 19, 2021 at 6:28 AM Alice Guo (OSS) <alice.guo@oss.nxp.com> wrote: > > From: Alice Guo <alice.guo@nxp.com> > > > > In i.MX8M boards, the registration of SoC device is later than caam > > driver which needs it. Caam driver needs soc_device_match to provide > > -EPROBE_DEFER when no SoC device is registered and no > > early_soc_dev_attr. > > I'm wondering if this is really a good idea: soc_device_match() is a > last-resort low-level check, and IMHO should be made available early on, > so there is no need for -EPROBE_DEFER. > > > > > Signed-off-by: Alice Guo <alice.guo@nxp.com> > > Thanks for your patch! > > > --- a/drivers/base/soc.c > > +++ b/drivers/base/soc.c > > @@ -110,6 +110,7 @@ static void soc_release(struct device *dev) > > } > > > > static struct soc_device_attribute *early_soc_dev_attr; > > +static bool soc_dev_attr_init_done = false; > > Do you need this variable? > > > > > struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr) > > { > > @@ -157,6 +158,7 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr > > return ERR_PTR(ret); > > } > > > > + soc_dev_attr_init_done = true; > > return soc_dev; > > > > out3: > > @@ -246,6 +248,9 @@ const struct soc_device_attribute *soc_device_match( > > if (!matches) > > return NULL; > > > > + if (!soc_dev_attr_init_done && !early_soc_dev_attr) > > if (!soc_bus_type.p && !early_soc_dev_attr) There is one place checking this already. We could wrap it in a helper function: static bool device_init_done(void) { return soc_bus_type.p ? true : false; } regards, dan carpenter
diff --git a/drivers/base/soc.c b/drivers/base/soc.c index 0af5363a582c..12a22f9cf5c8 100644 --- a/drivers/base/soc.c +++ b/drivers/base/soc.c @@ -110,6 +110,7 @@ static void soc_release(struct device *dev) } static struct soc_device_attribute *early_soc_dev_attr; +static bool soc_dev_attr_init_done = false; struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr) { @@ -157,6 +158,7 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr return ERR_PTR(ret); } + soc_dev_attr_init_done = true; return soc_dev; out3: @@ -246,6 +248,9 @@ const struct soc_device_attribute *soc_device_match( if (!matches) return NULL; + if (!soc_dev_attr_init_done && !early_soc_dev_attr) + return ERR_PTR(-EPROBE_DEFER); + while (!ret) { if (!(matches->machine || matches->family || matches->revision || matches->soc_id))