linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] AUXADC: Mediatek auxadc driver
@ 2021-07-15  9:35 Hui Liu
  2021-07-15  9:35 ` [PATCH] iio: mtk-auxadc: add mutex_destroy Hui Liu
  0 siblings, 1 reply; 5+ messages in thread
From: Hui Liu @ 2021-07-15  9:35 UTC (permalink / raw)
  To: robh+dt, jic23, knaack.h, lars, pmeerw
  Cc: srv_heupstream, hui.liu, zhiyong.tao, chun-hung.wu, yingjoe.chen,
	seiya.wang, matthias.bgg, s.hauer, devicetree, linux-kernel,
	linux-arm-kernel, linux-iio, linux-mediatek

This series includes one patch:
1.Add mutex_destroy when probe fail and remove device.

Hui Liu (1):
  iio: mtk-auxadc: add mutex_destroy

 drivers/iio/adc/mt6577_auxadc.c | 2 ++
 1 file changed, 2 insertions(+)

--
2.18.0



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] iio: mtk-auxadc: add mutex_destroy
  2021-07-15  9:35 [PATCH 0/1] AUXADC: Mediatek auxadc driver Hui Liu
@ 2021-07-15  9:35 ` Hui Liu
  2021-07-17 16:44   ` Jonathan Cameron
  0 siblings, 1 reply; 5+ messages in thread
From: Hui Liu @ 2021-07-15  9:35 UTC (permalink / raw)
  To: robh+dt, jic23, knaack.h, lars, pmeerw
  Cc: srv_heupstream, hui.liu, zhiyong.tao, chun-hung.wu, yingjoe.chen,
	seiya.wang, matthias.bgg, s.hauer, devicetree, linux-kernel,
	linux-arm-kernel, linux-iio, linux-mediatek

Add mutex_destroy when probe fail and remove device.

Signed-off-by: Hui Liu <hui.liu@mediatek.com>
---
 drivers/iio/adc/mt6577_auxadc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/adc/mt6577_auxadc.c b/drivers/iio/adc/mt6577_auxadc.c
index 79c1dd68b909..d57243037ad6 100644
--- a/drivers/iio/adc/mt6577_auxadc.c
+++ b/drivers/iio/adc/mt6577_auxadc.c
@@ -289,6 +289,7 @@ static int mt6577_auxadc_probe(struct platform_device *pdev)
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to register iio device\n");
+		mutex_destroy(&adc_dev->lock);
 		goto err_power_off;
 	}
 
@@ -313,6 +314,7 @@ static int mt6577_auxadc_remove(struct platform_device *pdev)
 			      0, MT6577_AUXADC_PDN_EN);
 
 	clk_disable_unprepare(adc_dev->adc_clk);
+	mutex_destroy(&adc_dev->lock);
 
 	return 0;
 }
-- 
2.18.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio: mtk-auxadc: add mutex_destroy
  2021-07-15  9:35 ` [PATCH] iio: mtk-auxadc: add mutex_destroy Hui Liu
@ 2021-07-17 16:44   ` Jonathan Cameron
       [not found]     ` <1627042875.27985.15.camel@mhfsdcap03>
  0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2021-07-17 16:44 UTC (permalink / raw)
  To: Hui Liu
  Cc: robh+dt, knaack.h, lars, pmeerw, srv_heupstream, zhiyong.tao,
	chun-hung.wu, yingjoe.chen, seiya.wang, matthias.bgg, s.hauer,
	devicetree, linux-kernel, linux-arm-kernel, linux-iio,
	linux-mediatek

On Thu, 15 Jul 2021 17:35:23 +0800
Hui Liu <hui.liu@mediatek.com> wrote:

> Add mutex_destroy when probe fail and remove device.
> 
> Signed-off-by: Hui Liu <hui.liu@mediatek.com>
Hi Hui Liu,

We very very rarely bother to call mutex_destroy().  The reason is
that it is only a non noop in when mutex debugging is enabled and
that is only useful if there is a plausible route in which it could
be used after the mutex_destroy.   Given these are both at the ends
of removal paths, I don't think this is useful.  That's why you will
rarely find mutex_destroy() being called.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/mt6577_auxadc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/adc/mt6577_auxadc.c b/drivers/iio/adc/mt6577_auxadc.c
> index 79c1dd68b909..d57243037ad6 100644
> --- a/drivers/iio/adc/mt6577_auxadc.c
> +++ b/drivers/iio/adc/mt6577_auxadc.c
> @@ -289,6 +289,7 @@ static int mt6577_auxadc_probe(struct platform_device *pdev)
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to register iio device\n");
> +		mutex_destroy(&adc_dev->lock);
>  		goto err_power_off;
>  	}
>  
> @@ -313,6 +314,7 @@ static int mt6577_auxadc_remove(struct platform_device *pdev)
>  			      0, MT6577_AUXADC_PDN_EN);
>  
>  	clk_disable_unprepare(adc_dev->adc_clk);
> +	mutex_destroy(&adc_dev->lock);
>  
>  	return 0;
>  }


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio: mtk-auxadc: add mutex_destroy
       [not found]     ` <1627042875.27985.15.camel@mhfsdcap03>
@ 2021-07-24 17:30       ` Jonathan Cameron
       [not found]         ` <1627300994.11261.11.camel@mhfsdcap03>
  0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2021-07-24 17:30 UTC (permalink / raw)
  To: hui.liu
  Cc: robh+dt, knaack.h, lars, pmeerw, srv_heupstream, zhiyong.tao,
	chun-hung.wu, yingjoe.chen, seiya.wang, matthias.bgg, s.hauer,
	devicetree, linux-kernel, linux-arm-kernel, linux-iio,
	linux-mediatek

On Fri, 23 Jul 2021 20:21:15 +0800
hui.liu <hui.liu@mediatek.com> wrote:

> On Sat, 2021-07-17 at 17:44 +0100, Jonathan Cameron wrote:
> > On Thu, 15 Jul 2021 17:35:23 +0800
> > Hui Liu <hui.liu@mediatek.com> wrote:
> >   
> > > Add mutex_destroy when probe fail and remove device.
> > > 
> > > Signed-off-by: Hui Liu <hui.liu@mediatek.com>  
> > Hi Hui Liu,
> > 
> > We very very rarely bother to call mutex_destroy().  The reason is
> > that it is only a non noop in when mutex debugging is enabled and
> > that is only useful if there is a plausible route in which it could
> > be used after the mutex_destroy.   Given these are both at the ends
> > of removal paths, I don't think this is useful.  That's why you will
> > rarely find mutex_destroy() being called.
> > 
> > Thanks,
> > 
> > Jonathan  
> 
> Hi Jonathon,
> 
> I think this patch could assurance the integrity of code.
> mutex_init will be called when driver probe. If driver probe fail or
> device removed, mutex_destroy could set lock->magic to NULL.

I'm not seeing the use case here given the location doesn't leave
a huge amount of code that could have such a bug.  There might have been
something if we had any route to increment the reference count of the
structure this mutex is ultimately embedded in and so have it outlast
the remove function or error path. In this driver it looks like there is
no such path.  Hence you are protecting against a automated
cleanup of core code (nothing in the driver itself) which is obviously
not going to try taking a driver specific mutex.

A few side notes:

You are calling it wrong place in remove. The ordering in remove
should be the opposite of that in probe so the mutex_destroy should either
be a few lines earlier, or you should have a comment there to say why you
are doing it where you have chosen to do so.

The style of this probe is to do error handling in a block at the end.
So this handling should be there, not in the if statement.

Jonathan



> 
> Thanks.
> Hui
> 
> >   
> > > ---
> > >  drivers/iio/adc/mt6577_auxadc.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/iio/adc/mt6577_auxadc.c b/drivers/iio/adc/mt6577_auxadc.c
> > > index 79c1dd68b909..d57243037ad6 100644
> > > --- a/drivers/iio/adc/mt6577_auxadc.c
> > > +++ b/drivers/iio/adc/mt6577_auxadc.c
> > > @@ -289,6 +289,7 @@ static int mt6577_auxadc_probe(struct platform_device *pdev)
> > >  	ret = iio_device_register(indio_dev);
> > >  	if (ret < 0) {
> > >  		dev_err(&pdev->dev, "failed to register iio device\n");
> > > +		mutex_destroy(&adc_dev->lock);
> > >  		goto err_power_off;
> > >  	}
> > >  
> > > @@ -313,6 +314,7 @@ static int mt6577_auxadc_remove(struct platform_device *pdev)
> > >  			      0, MT6577_AUXADC_PDN_EN);
> > >  
> > >  	clk_disable_unprepare(adc_dev->adc_clk);
> > > +	mutex_destroy(&adc_dev->lock);
> > >  
> > >  	return 0;
> > >  }  
> >   
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio: mtk-auxadc: add mutex_destroy
       [not found]         ` <1627300994.11261.11.camel@mhfsdcap03>
@ 2021-07-31 17:52           ` Jonathan Cameron
  0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron @ 2021-07-31 17:52 UTC (permalink / raw)
  To: hui.liu
  Cc: robh+dt, lars, pmeerw, srv_heupstream, zhiyong.tao, chun-hung.wu,
	yingjoe.chen, seiya.wang, matthias.bgg, s.hauer, devicetree,
	linux-kernel, linux-arm-kernel, linux-iio, linux-mediatek

On Mon, 26 Jul 2021 20:03:14 +0800
hui.liu <hui.liu@mediatek.com> wrote:

> On Sat, 2021-07-24 at 18:30 +0100, Jonathan Cameron wrote:
> > On Fri, 23 Jul 2021 20:21:15 +0800
> > hui.liu <hui.liu@mediatek.com> wrote:
> >   
> > > On Sat, 2021-07-17 at 17:44 +0100, Jonathan Cameron wrote:  
> > > > On Thu, 15 Jul 2021 17:35:23 +0800
> > > > Hui Liu <hui.liu@mediatek.com> wrote:
> > > >     
> > > > > Add mutex_destroy when probe fail and remove device.
> > > > > 
> > > > > Signed-off-by: Hui Liu <hui.liu@mediatek.com>    
> > > > Hi Hui Liu,
> > > > 
> > > > We very very rarely bother to call mutex_destroy().  The reason is
> > > > that it is only a non noop in when mutex debugging is enabled and
> > > > that is only useful if there is a plausible route in which it could
> > > > be used after the mutex_destroy.   Given these are both at the ends
> > > > of removal paths, I don't think this is useful.  That's why you will
> > > > rarely find mutex_destroy() being called.
> > > > 
> > > > Thanks,
> > > > 
> > > > Jonathan    
> > > 
> > > Hi Jonathon,
> > > 
> > > I think this patch could assurance the integrity of code.
> > > mutex_init will be called when driver probe. If driver probe fail or
> > > device removed, mutex_destroy could set lock->magic to NULL.  
> > 
> > I'm not seeing the use case here given the location doesn't leave
> > a huge amount of code that could have such a bug.  There might have been
> > something if we had any route to increment the reference count of the
> > structure this mutex is ultimately embedded in and so have it outlast
> > the remove function or error path. In this driver it looks like there is
> > no such path.  Hence you are protecting against a automated
> > cleanup of core code (nothing in the driver itself) which is obviously
> > not going to try taking a driver specific mutex.
> > 
> > A few side notes:
> > 
> > You are calling it wrong place in remove. The ordering in remove
> > should be the opposite of that in probe so the mutex_destroy should either
> > be a few lines earlier, or you should have a comment there to say why you
> > are doing it where you have chosen to do so.
> > 
> > The style of this probe is to do error handling in a block at the end.
> > So this handling should be there, not in the if statement.
> > 
> > Jonathan
> > 
> >   
> Hi Jonathon,
> 
> Base on your helpful opinion, We will to do two changes in patch v2.
> 1. In probe: move mutex_destroy from the if statement to error handling
> path(err_power_off).
> 2. In remove: calling mutex_destroy right after iio_device_unregister.
> 
> Do we need some more change? Thanks.
Ah. Sorry I missed this in the flood of emails during the week.

Anyhow, I've replied to the v1 posting.

Jonathan

> >   
> > > 
> > > Thanks.
> > > Hui
> > >   
> > > >     
> > > > > ---
> > > > >  drivers/iio/adc/mt6577_auxadc.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/iio/adc/mt6577_auxadc.c b/drivers/iio/adc/mt6577_auxadc.c
> > > > > index 79c1dd68b909..d57243037ad6 100644
> > > > > --- a/drivers/iio/adc/mt6577_auxadc.c
> > > > > +++ b/drivers/iio/adc/mt6577_auxadc.c
> > > > > @@ -289,6 +289,7 @@ static int mt6577_auxadc_probe(struct platform_device *pdev)
> > > > >  	ret = iio_device_register(indio_dev);
> > > > >  	if (ret < 0) {
> > > > >  		dev_err(&pdev->dev, "failed to register iio device\n");
> > > > > +		mutex_destroy(&adc_dev->lock);
> > > > >  		goto err_power_off;
> > > > >  	}
> > > > >  
> > > > > @@ -313,6 +314,7 @@ static int mt6577_auxadc_remove(struct platform_device *pdev)
> > > > >  			      0, MT6577_AUXADC_PDN_EN);
> > > > >  
> > > > >  	clk_disable_unprepare(adc_dev->adc_clk);
> > > > > +	mutex_destroy(&adc_dev->lock);
> > > > >  
> > > > >  	return 0;
> > > > >  }    
> > > >     
> > >   
> >   
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-07-31 17:49 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-15  9:35 [PATCH 0/1] AUXADC: Mediatek auxadc driver Hui Liu
2021-07-15  9:35 ` [PATCH] iio: mtk-auxadc: add mutex_destroy Hui Liu
2021-07-17 16:44   ` Jonathan Cameron
     [not found]     ` <1627042875.27985.15.camel@mhfsdcap03>
2021-07-24 17:30       ` Jonathan Cameron
     [not found]         ` <1627300994.11261.11.camel@mhfsdcap03>
2021-07-31 17:52           ` Jonathan Cameron

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).