spin_lock_irqsave first used and then unused

[Torin Carey]

Hi Philipp,

The code might be taking advantage of the fact that on some architectures,
reads and writes to aligned words are atomic, meaning the spinlock might
not be required for the loop.

>From the snippet, it looks like a mutual-exclusion mechanism—only one thread
of execution is allowed to set rf_change_in_progress to true, and the other
threads will retry with a timeout (but it could theoretially retry forever, since
this timeout resets whenever it notices rf_change_in_progress turn false).

When a thread wants to acquire this rf_change_in_progress "lock", it needs
to check that it's currently false so it can change it to true, but without
the lock serialising the writes, multiple threads could see it be rf_change_in_progress
be false, and then change it to true.  The spinlock serialises these attempts.

Checking rf_change_in_progress without holding the spinlock isn't necessarily
invalid (RCU works on a similar principle), but it doesn't guarantee that
once we do acquire the spinlock, that rf_change_in_progress would have the
same value.

Torin

On Sun, Apr 30, 2023 at 05:57:41PM +0200, Philipp Hortmann wrote:
> Hi Frank,
>
> On 4/30/23 15:20, Frank Oltmanns wrote:
> > Hi Philipp,
> >
> > I'm a kernel newbie myself (but a senior software developer) and I
> > haven't actually looked up the source, other then the part you cited, so
> > please take the following with a grain of salt.
> >
> > On 2023-04-30 at 10:31:09 +0200, Philipp Hortmann <philipp.g.hortmann at gmail.com> wrote:
> >> Hi,
> >>
> >> here a piece of code from driver rtl8192e:
> >>
> >> 	while (true) {
> >> 		spin_lock_irqsave(&priv->rf_ps_lock, flag);
> >> 		if (priv->rf_change_in_progress) {
> >> 			spin_unlock_irqrestore(&priv->rf_ps_lock, flag);
> >>
> >> 			while (priv->rf_change_in_progress) {
> >> 				rf_wait_counter++;
> >> 				mdelay(1);
> >>
> >> 				if (rf_wait_counter > 100) {
> >> 					netdev_warn(dev,
> >> 						    "%s(): Timeout waiting for RF change.\n",
> >> 						    __func__);
> >> 					return false;
> >> 				}
> >> 			}
> >> 		} else {
> >> 			priv->rf_change_in_progress = true;
> >> 			spin_unlock_irqrestore(&priv->rf_ps_lock, flag);
> >> 			break;
> >> 		}
> >> 	}
> >>
> >> For me something is wrong here. First the access of priv->rf_change_in_progress
> >> is protected by a spin lock and then in the while loop it is unprotected. Is
> >> this correct? For me it is required to protected it always or protected it
> >> never.
> >
> > The lock pertains to the else-branch. I.e. you need a lock when writing
> > to priv->rf_change_in_progress.
> >
>
> thank you very much for your quick response.
> So is the spin_lock not needed for reading in this case?
> I expect a lock for the following line:
> 		while (priv->rf_change_in_progress) {
>
>
> When only the writing needs to be protected why the else part is not
> looking like this:
> 		spin_lock_irqsave(&priv->rf_ps_lock, flag);
> 		priv->rf_change_in_progress = true;
> 		spin_unlock_irqrestore(&priv->rf_ps_lock, flag);
>
>
> Feel free to contact me for questions regarding kernel patches.
>
> Feel free to support me with patches on the driver I am working on:
> drivers/staging/rtl8192e/
> I can then give you a "Tested-by:"
>
> Thanks for your support.
>
> Bye Philipp
>
>
>
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies