* [PATCH] Update spmi driver removal API @ 2022-12-04 9:22 Jishnu Prakash 2022-12-04 9:23 ` [PATCH] spmi: Add check for remove callback in spmi_drv_remove API Jishnu Prakash 0 siblings, 1 reply; 6+ messages in thread From: Jishnu Prakash @ 2022-12-04 9:22 UTC (permalink / raw) To: agross, bjorn.andersson, devicetree, robh+dt, linus.walleij, quic_kamalw, quic_jestar, sboyd, quic_subbaram, quic_collinsd Cc: linux-arm-msm, linux-kernel, linux-arm-msm-owner, Jishnu Prakash Add a check for remove callback presence before calling it, to avoid NULL pointer dereference error when attempting to remove a SPMI driver module which does not have the remove callback defined. Jishnu Prakash (1): spmi: Add check for remove callback in spmi_drv_remove API drivers/spmi/spmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] spmi: Add check for remove callback in spmi_drv_remove API 2022-12-04 9:22 [PATCH] Update spmi driver removal API Jishnu Prakash @ 2022-12-04 9:23 ` Jishnu Prakash 2022-12-13 12:04 ` Greg KH 0 siblings, 1 reply; 6+ messages in thread From: Jishnu Prakash @ 2022-12-04 9:23 UTC (permalink / raw) To: agross, bjorn.andersson, devicetree, robh+dt, linus.walleij, quic_kamalw, quic_jestar, sboyd, quic_subbaram, quic_collinsd, linux-kernel Cc: linux-arm-msm, linux-arm-msm-owner, Jishnu Prakash Add a check for remove callback presence before calling it for a spmi driver, to avoid NULL pointer dereference error if remove callback has not been specified for that SPMI driver. Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com> --- drivers/spmi/spmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c index a456ce5..6b34356 100644 --- a/drivers/spmi/spmi.c +++ b/drivers/spmi/spmi.c @@ -350,7 +350,8 @@ static void spmi_drv_remove(struct device *dev) const struct spmi_driver *sdrv = to_spmi_driver(dev->driver); pm_runtime_get_sync(dev); - sdrv->remove(to_spmi_device(dev)); + if (sdrv->remove) + sdrv->remove(to_spmi_device(dev)); pm_runtime_put_noidle(dev); pm_runtime_disable(dev); -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] spmi: Add check for remove callback in spmi_drv_remove API 2022-12-04 9:23 ` [PATCH] spmi: Add check for remove callback in spmi_drv_remove API Jishnu Prakash @ 2022-12-13 12:04 ` Greg KH 2022-12-13 13:42 ` Jishnu Prakash 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2022-12-13 12:04 UTC (permalink / raw) To: Jishnu Prakash Cc: agross, bjorn.andersson, devicetree, robh+dt, linus.walleij, quic_kamalw, quic_jestar, sboyd, quic_subbaram, quic_collinsd, linux-kernel, linux-arm-msm, linux-arm-msm-owner On Sun, Dec 04, 2022 at 02:53:00PM +0530, Jishnu Prakash wrote: > Add a check for remove callback presence before calling it for a > spmi driver, to avoid NULL pointer dereference error if remove callback > has not been specified for that SPMI driver. > > Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com> > --- > drivers/spmi/spmi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c > index a456ce5..6b34356 100644 > --- a/drivers/spmi/spmi.c > +++ b/drivers/spmi/spmi.c > @@ -350,7 +350,8 @@ static void spmi_drv_remove(struct device *dev) > const struct spmi_driver *sdrv = to_spmi_driver(dev->driver); > > pm_runtime_get_sync(dev); > - sdrv->remove(to_spmi_device(dev)); > + if (sdrv->remove) > + sdrv->remove(to_spmi_device(dev)); > pm_runtime_put_noidle(dev); > > pm_runtime_disable(dev); What in-kernel spmi driver does not have a remove function set that requires this change? thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spmi: Add check for remove callback in spmi_drv_remove API 2022-12-13 12:04 ` Greg KH @ 2022-12-13 13:42 ` Jishnu Prakash 2022-12-13 15:09 ` Greg KH 0 siblings, 1 reply; 6+ messages in thread From: Jishnu Prakash @ 2022-12-13 13:42 UTC (permalink / raw) To: Greg KH Cc: agross, bjorn.andersson, devicetree, robh+dt, linus.walleij, quic_kamalw, quic_jestar, sboyd, quic_subbaram, quic_collinsd, linux-kernel, linux-arm-msm, linux-arm-msm-owner Hi Greg These are two SPMI drivers without remove callbacks defined: drivers/mfd/qcom-spmi-pmic.c drivers/mfd/hi6421-spmi-pmic.c We made this change after noticing an issue internally with the first one above, there was a crash when trying to remove it with rmmod, which is fixed by this change. In addition, since the probe of the QCOM SPMI PMIC driver uses devm_ functions throughout, we could see that with this change, when we remove the device with rmmod, the cleanup does happen correctly even though there is no remove function defined in the driver. The last function called in the probe of our SPMI PMIC driver is devm_of_platform_populate(), to probe all the PMIC peripheral drivers under this one, and when this driver module was removed with rmmod, we could see that the individual PMIC drivers under it also got depopulated with their remove APIs getting called. If it is possible for a SPMI driver to be removed correctly by rmmod without having a remove API defined, this change should be right, what do you think? Thanks, Jishnu On 12/13/2022 5:34 PM, Greg KH wrote: > On Sun, Dec 04, 2022 at 02:53:00PM +0530, Jishnu Prakash wrote: >> Add a check for remove callback presence before calling it for a >> spmi driver, to avoid NULL pointer dereference error if remove callback >> has not been specified for that SPMI driver. >> >> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com> >> --- >> drivers/spmi/spmi.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c >> index a456ce5..6b34356 100644 >> --- a/drivers/spmi/spmi.c >> +++ b/drivers/spmi/spmi.c >> @@ -350,7 +350,8 @@ static void spmi_drv_remove(struct device *dev) >> const struct spmi_driver *sdrv = to_spmi_driver(dev->driver); >> >> pm_runtime_get_sync(dev); >> - sdrv->remove(to_spmi_device(dev)); >> + if (sdrv->remove) >> + sdrv->remove(to_spmi_device(dev)); >> pm_runtime_put_noidle(dev); >> >> pm_runtime_disable(dev); > > What in-kernel spmi driver does not have a remove function set that > requires this change? > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spmi: Add check for remove callback in spmi_drv_remove API 2022-12-13 13:42 ` Jishnu Prakash @ 2022-12-13 15:09 ` Greg KH 2022-12-21 5:39 ` Jishnu Prakash 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2022-12-13 15:09 UTC (permalink / raw) To: Jishnu Prakash Cc: agross, bjorn.andersson, devicetree, robh+dt, linus.walleij, quic_kamalw, quic_jestar, sboyd, quic_subbaram, quic_collinsd, linux-kernel, linux-arm-msm, linux-arm-msm-owner On Tue, Dec 13, 2022 at 07:12:10PM +0530, Jishnu Prakash wrote: > Hi Greg Hi, please do not top-post :( > These are two SPMI drivers without remove callbacks defined: > > drivers/mfd/qcom-spmi-pmic.c > drivers/mfd/hi6421-spmi-pmic.c Great, they should be fixed up now, right? > We made this change after noticing an issue internally with the first one > above, there was a crash when trying to remove it with rmmod, which is fixed > by this change. Then please say that in the changelog text, otherwise we have no idea _why_ this is needed. All you said was "add this new check _IF_" and we have no idea what the answer to "if" is :( thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] spmi: Add check for remove callback in spmi_drv_remove API 2022-12-13 15:09 ` Greg KH @ 2022-12-21 5:39 ` Jishnu Prakash 0 siblings, 0 replies; 6+ messages in thread From: Jishnu Prakash @ 2022-12-21 5:39 UTC (permalink / raw) To: Greg KH Cc: agross, devicetree, robh+dt, linus.walleij, quic_kamalw, sboyd, quic_subbaram, quic_collinsd, linux-kernel, linux-arm-msm, linux-arm-msm-owner Hi Greg On 12/13/2022 8:39 PM, Greg KH wrote: > On Tue, Dec 13, 2022 at 07:12:10PM +0530, Jishnu Prakash wrote: >> Hi Greg > > Hi, please do not top-post :( > >> These are two SPMI drivers without remove callbacks defined: >> >> drivers/mfd/qcom-spmi-pmic.c >> drivers/mfd/hi6421-spmi-pmic.c > > Great, they should be fixed up now, right? > Our QCOM SPMI PMIC driver allocates resources in its probe using only devm_() APIs and does not require any other cleanup. It doesn't seem right to add an empty remove callback to it just to avoid this crash, it seems the better solution architecturally is to call the remove function only if it's defined. In addition, I could see that other buses like PCI and AMBA do check for the remove API being defined for their drivers before calling it: AMBA example: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/amba/bus.c#n328 PCI example: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci-driver.c#n474 >> We made this change after noticing an issue internally with the first one >> above, there was a crash when trying to remove it with rmmod, which is fixed >> by this change. > > Then please say that in the changelog text, otherwise we have no idea > _why_ this is needed. All you said was "add this new check _IF_" and we > have no idea what the answer to "if" is :( > I have uploaded the change with an updated title and commit text, can you please have a look? > thanks, > > greg k-h Thanks, Jishnu ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-21 5:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-04 9:22 [PATCH] Update spmi driver removal API Jishnu Prakash 2022-12-04 9:23 ` [PATCH] spmi: Add check for remove callback in spmi_drv_remove API Jishnu Prakash 2022-12-13 12:04 ` Greg KH 2022-12-13 13:42 ` Jishnu Prakash 2022-12-13 15:09 ` Greg KH 2022-12-21 5:39 ` Jishnu Prakash
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).