spin_lock_irqsave first used and then unused

[Philipp Hortmann]

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