[PATCH v3 4/9] mtd: spi-nor: Reorder the preparation vs locking steps

[Miquel Raynal]

Hi Tudor,

tudor.ambarus at linaro.org wrote on Tue, 31 Jan 2023 05:11:33 +0000:

> Hi, Miquel,
> 
> On 12/15/22 08:12, Miquel Raynal wrote:
> > The ->prepare()/->unprepare() hooks are now legacy, and there are only
> > two controllers left supporting them. In both cases, the implementation  
> 
> now there's only one, hisi-sfc.c.

Indeed.

> 
> > acquires a mutex, which is somehow redundant with the spi-nor main lock  
> 
> I see a HIFMC_MAX_CHIP_NUM with value 2, the controller seems to be able
> to operate 2 flashes in parallel and that's why the internal mutex.
> 
> > that we acquire as well in the spi_nor_[un]lock_and_[un]prep() helpers.
> > 
> > While the mutex taken in the core is necessary, the helper can be
> > reorganized to first do the preparation, then acquire the core
> > lock. This is necessary in order to be able to improve the locking
> > mechanism in the core and should have no side effect.
> >   
> 
> the change seems fine for hisi, and since we're no longer adding drivers
> under spi-nor/controllers we should be good.
> 
> > Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> > ---
> >   drivers/mtd/spi-nor/core.c | 15 ++++++---------
> >   1 file changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 38a57aac6754..de77ca55f74d 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -1074,23 +1074,20 @@ int spi_nor_lock_and_prep(struct spi_nor *nor)  
> 
> you'll have to rename it to spi_nor_prep_and_lock to reflect the order
> of ops.

Sure.

> 
> >   {
> >   	int ret = 0;  
> >   > -	mutex_lock(&nor->lock);  
> > -
> > -	if (nor->controller_ops &&  nor->controller_ops->prepare) {
> > +	if (nor->controller_ops && nor->controller_ops->prepare)
> >   		ret = nor->controller_ops->prepare(nor);
> > -		if (ret) {
> > -			mutex_unlock(&nor->lock);
> > -			return ret;
> > -		}
> > -	}
> > +
> > +	mutex_lock(&nor->lock);
> > +
> >   	return ret;
> >   }
> >   
> 
> Cheers,
> ta


Thanks,
Miquèl