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@xxxxxxxxx> 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
