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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 24 Nov 2023 13:22:36 +0200
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     Hugo Villeneuve <hugo@...ovil.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        lech.perczak@...lingroup.com,
        Hugo Villeneuve <hvilleneuve@...onoff.com>,
        linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org
Subject: Re: [PATCH] serial: sc16is7xx: improve regmap debugfs by using one
 regmap per port

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@...el.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