Re: [cip-dev] [PATCH for 5.10.y] phy: renesas: rcar-gen3-usb2: Fix the use of msleep during spinlock

[Pavel Machek <pavel@nabladev.com>]

[-- Attachment #1: Type: text/plain, Size: 2074 bytes --]

Hi!

> > > > > From: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
> > > > > 
> > > > > This fixes an issue caused by the use of msleep during spinlock.
> > > > > In the original commit, msleep was changed to mdelay, but this fix was not
> > > > > carried over during the backport to 5.10.y tree.
> > > > 
> > > > Doing this as a quick fix is probably okay, but this should not be
> > > > final version.
> > > > 
> > > > You are right that msleep inside spinlock will blow up immediately:
> > > > 
> > > > > ```
> > > > > [   62.677594] BUG: scheduling while atomic: kworker/1:2/126/0x00000002
> > > > > [   62.683957] Modules linked in:
> > > > 
> > > > But mdelay for 20 msec inside irqsave spinlock is borderline
> > > > unacceptable, too.
...
> > > I'm going to investigate and come with a better approach for this.
> > 
> > On a second thought, the proper fix would have to go to upstream first. To
> > fix the problem currently seen, could we integrate this patch as is?
> 
> No, please work upstream first.

Upstream has the mdelay:

drivers/phy/renesas/phy-rcar-gen3-usb2.c in rcar_gen3_init_otg() --
mdelay(20) and has the spinlock,

static int rcar_gen3_phy_usb2_init(struct phy *p)
{
	...
        guard(spinlock_irqsave)(&channel->lock);
	...
        /* Initialize otg part (only if we initialize a PHY with IRQs). */
        if (rphy->int_enable_bits)
                rcar_gen3_init_otg(channel);


So what Iwamatsu-san's patch matches upstream, and is good solution
for -stable for now. I have just pointed out, that, well, mdelay(20)
inside a spinlock is not great.

7.1-rc2: mdelay inside spinlock -- not great

5.10.250: msleep but no spinlock -- missing locking, bad!

5.10.254: msleep inside spinlock -- fails to boot, bad, bad, BAD! (and
also regression from 5.10.250).

Please allow Iwamatsu-san / Renesas to get this to 7.1 level of "not
great" situation; Claudiu will work with upstream to get 7.1 to "good"
situation, and then we can thing about backports.

Thanks and best regards,
								Pavel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]