Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Fri, 24 Nov 2023 13:22:36 +0200
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     Hugo Villeneuve <hugo@hugovil.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        lech.perczak@camlingroup.com,
        Hugo Villeneuve <hvilleneuve@dimonoff.com>,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
Subject: Re: [PATCH] serial: sc16is7xx: improve regmap debugfs by using one
 regmap per port
Message-ID: <ZWCHfGmAmSpGh2e1@smile.fi.intel.com>
References: <20231030211447.974779-1-hugo@hugovil.com>
 <ZV_GHRhqCdeCHV_a@smile.fi.intel.com>
 <20231124000534.aa8f0c866753c3a9e6844354@hugovil.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231124000534.aa8f0c866753c3a9e6844354@hugovil.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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