LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: "hui.liu" <hui.liu@mediatek.com>
Cc: <robh+dt@kernel.org>, <lars@metafoo.de>, <pmeerw@pmeerw.net>,
	<srv_heupstream@mediatek.com>, <zhiyong.tao@mediatek.com>,
	<chun-hung.wu@mediatek.com>, <yingjoe.chen@mediatek.com>,
	<seiya.wang@mediatek.com>, <matthias.bgg@gmail.com>,
	<s.hauer@pengutronix.de>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-iio@vger.kernel.org>, <linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH] iio: mtk-auxadc: add mutex_destroy
Date: Sat, 31 Jul 2021 18:52:10 +0100	[thread overview]
Message-ID: <20210731185210.7acb5f79@jic23-huawei> (raw)
In-Reply-To: <1627300994.11261.11.camel@mhfsdcap03>

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;
> > > > >  }    
> > > >     
> > >   
> >   
> 


      parent reply	other threads:[~2021-07-31 17:49 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210731185210.7acb5f79@jic23-huawei \
    --to=jic23@kernel.org \
    --cc=chun-hung.wu@mediatek.com \
    --cc=devicetree@vger.kernel.org \
    --cc=hui.liu@mediatek.com \
    --cc=lars@metafoo.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=pmeerw@pmeerw.net \
    --cc=robh+dt@kernel.org \
    --cc=s.hauer@pengutronix.de \
    --cc=seiya.wang@mediatek.com \
    --cc=srv_heupstream@mediatek.com \
    --cc=yingjoe.chen@mediatek.com \
    --cc=zhiyong.tao@mediatek.com \
    --subject='Re: [PATCH] iio: mtk-auxadc: add mutex_destroy' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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