Re: [PATCH] spmi: Add check for remove callback in spmi_drv_remove API

[Jishnu Prakash <quic_jprakash@quicinc.com>]

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