Re: [PATCH] serial: sc16is7xx: improve regmap debugfs by using one regmap per port

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Nov 24, 2023 at 12:05:34AM -0500, Hugo Villeneuve wrote:
> On Thu, 23 Nov 2023 23:37:33 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Mon, Oct 30, 2023 at 05:14:47PM -0400, Hugo Villeneuve wrote:

...

> > This change might be problematic, i.e. ...

...

> > >  		regmap_update_bits(
> > >  			s->regmap,
> > > -			SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> > > +			SC16IS7XX_IOCONTROL_REG,
> > >  			SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> > >  			SC16IS7XX_IOCONTROL_MODEM_B_BIT, s->mctrl_mask);
> > 
> > ...if this happens inside another regmap operation it might collide with this
> > as there is no more shared locking (and if driver is going to be converted to
> > use an external lock, the one in regmap might be disabled). But I haven't
> > checked anyhow deeply this, so just a heads up for the potential issue.
> 
> Hi Andy,
> are you refering to the above piece of code as the only location where
> this could be problematic?
> 
> If yes, then it is located inside sc16is7xx_setup_mctrl_ports(), which
> is called only during sc16is7xx_probe(), and I assume it should be ok.

With below it becomes two. Maybe you can point out somewhere in the code
(in a form of a comment?) that regmap[0] separate access is allowed only
in probe stage?

Also be aware, that other callbacks shouldn't be called at that time (means
no port should be made visible / registered to the users).

...

> > > -	ret = regmap_read(regmap,
> > > -			  SC16IS7XX_LSR_REG << SC16IS7XX_REG_SHIFT, &val);
> > > +	ret = regmap_read(regmaps[0], SC16IS7XX_LSR_REG, &val);
> > 
> > Here is a probe, most likely no issues.
> 
> Ok.
> 
> > >  	if (ret < 0)
> > >  		return -EPROBE_DEFER;

...

> > > +	snprintf(buf, sizeof(buf), "port%d", port_id);
> > 
> > Should be %u.
> 
> Yes. I just noticed that Greg has applied the patch to its tty-testing
> branch, I assume I should just send a new patch to fix it?

Yes.

-- 
With Best Regards,
Andy Shevchenko