| Message ID | 20230326143224.572654-5-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Sun, Mar 26, 2023 at 7:32 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Exiting early in remove without releasing all acquired resources yields > leaks. Note that e.g. memory allocated with devm_zalloc() is freed after > .remove() returns, even if the return code was negative. > > While blocking_notifier_chain_unregister() won't fail and so the > change is somewhat cosmetic, platform driver's .remove callbacks are > about to be converted to return void. To prepare that, keep the error > message but don't return early. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Reviewed-by: Guenter Roeck <groeck@chromium.org> > --- > drivers/media/cec/platform/cros-ec/cros-ec-cec.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > index 6ebedc71d67d..960432230bbf 100644 > --- a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > +++ b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > @@ -332,14 +332,16 @@ static int cros_ec_cec_remove(struct platform_device *pdev) > struct device *dev = &pdev->dev; > int ret; > > + /* > + * blocking_notifier_chain_unregister() only fails if the notifier isn't > + * in the list. We know it was added to it by .probe(), so there should > + * be no need for error checking. Be cautious and still check. > + */ > ret = blocking_notifier_chain_unregister( > &cros_ec_cec->cros_ec->event_notifier, > &cros_ec_cec->notifier); > - > - if (ret) { > + if (ret) > dev_err(dev, "failed to unregister notifier\n"); > - return ret; > - } > > cec_notifier_cec_adap_unregister(cros_ec_cec->notify, > cros_ec_cec->adap); > -- > 2.39.2 >
Just ignore this reply: for some reason this patch only ended up in my private mailbox, but not on linux-media (and therefor also not in patchwork). This should hopefully cause patchwork to pick it up. Regards, Hans On 26/03/2023 16:30, Uwe Kleine-König wrote: > Exiting early in remove without releasing all acquired resources yields > leaks. Note that e.g. memory allocated with devm_zalloc() is freed after > .remove() returns, even if the return code was negative. > > While blocking_notifier_chain_unregister() won't fail and so the > change is somewhat cosmetic, platform driver's .remove callbacks are > about to be converted to return void. To prepare that, keep the error > message but don't return early. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/media/cec/platform/cros-ec/cros-ec-cec.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > index 6ebedc71d67d..960432230bbf 100644 > --- a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > +++ b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c > @@ -332,14 +332,16 @@ static int cros_ec_cec_remove(struct platform_device *pdev) > struct device *dev = &pdev->dev; > int ret; > > + /* > + * blocking_notifier_chain_unregister() only fails if the notifier isn't > + * in the list. We know it was added to it by .probe(), so there should > + * be no need for error checking. Be cautious and still check. > + */ > ret = blocking_notifier_chain_unregister( > &cros_ec_cec->cros_ec->event_notifier, > &cros_ec_cec->notifier); > - > - if (ret) { > + if (ret) > dev_err(dev, "failed to unregister notifier\n"); > - return ret; > - } > > cec_notifier_cec_adap_unregister(cros_ec_cec->notify, > cros_ec_cec->adap);
diff --git a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c index 6ebedc71d67d..960432230bbf 100644 --- a/drivers/media/cec/platform/cros-ec/cros-ec-cec.c +++ b/drivers/media/cec/platform/cros-ec/cros-ec-cec.c @@ -332,14 +332,16 @@ static int cros_ec_cec_remove(struct platform_device *pdev) struct device *dev = &pdev->dev; int ret; + /* + * blocking_notifier_chain_unregister() only fails if the notifier isn't + * in the list. We know it was added to it by .probe(), so there should + * be no need for error checking. Be cautious and still check. + */ ret = blocking_notifier_chain_unregister( &cros_ec_cec->cros_ec->event_notifier, &cros_ec_cec->notifier); - - if (ret) { + if (ret) dev_err(dev, "failed to unregister notifier\n"); - return ret; - } cec_notifier_cec_adap_unregister(cros_ec_cec->notify, cros_ec_cec->adap);
Exiting early in remove without releasing all acquired resources yields leaks. Note that e.g. memory allocated with devm_zalloc() is freed after .remove() returns, even if the return code was negative. While blocking_notifier_chain_unregister() won't fail and so the change is somewhat cosmetic, platform driver's .remove callbacks are about to be converted to return void. To prepare that, keep the error message but don't return early. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/media/cec/platform/cros-ec/cros-ec-cec.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)