Re: [PATCH 1/2] scsi: Change scsi device boolean fields to single bit flags

[Damien Le Moal <dlemoal@kernel.org>]

On 11/21/23 01:06, Bart Van Assche wrote:
> On 11/19/23 23:35, Damien Le Moal wrote:
>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index 10480eb582b2..1fb460dfca0c 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -167,19 +167,19 @@ struct scsi_device {
>>   	 * power state for system suspend/resume (suspend to RAM and
>>   	 * hibernation) operations.
>>   	 */
>> -	bool manage_system_start_stop;
>> +	unsigned manage_system_start_stop:1;
>>   
>>   	/*
>>   	 * If true, let the high-level device driver (sd) manage the device
>>   	 * power state for runtime device suspand and resume operations.
>>   	 */
>> -	bool manage_runtime_start_stop;
>> +	unsigned manage_runtime_start_stop:1;
>>   
>>   	/*
>>   	 * If true, let the high-level device driver (sd) manage the device
>>   	 * power state for system shutdown (power off) operations.
>>   	 */
>> -	bool manage_shutdown;
>> +	unsigned manage_shutdown:1;
>>   
>>   	unsigned removable:1;
>>   	unsigned changed:1;	/* Data invalid due to media change */
> 
> Is there any code that modifies the above flags from different
> threads simultaneously? I'm wondering whether this patch introduces
> one or more race conditions related to changing these flags.

These are set once when the LLD probes and initialize the scsi device and never
changed again by the LLD. The manage_xxx_start_stop flags can be changed through
sysfs though, but doing so would be a mistake as things would stop working as
expected... I do think that we should change these flags to be RO in sysfs.

Note that only libata and the firewire/sbp2 driver use these flags.

> 
> Thanks,
> 
> Bart.

-- 
Damien Le Moal
Western Digital Research