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