Re: [PATCH v5 5/6] scsi: core: Protect host state changes with the host lock

[Bart Van Assche <bvanassche@acm.org>]

On 8/6/26 2:13 AM, John Garry wrote:
> On 05/08/2026 22:36, Bart Van Assche wrote:
>> @@ -785,11 +785,18 @@ static inline struct Scsi_Host 
>> *dev_to_shost(struct device *dev)
>>        return container_of(dev, struct Scsi_Host, shost_gendev);
>>    }
>> +static inline enum scsi_host_state scsi_get_host_state(struct 
>> Scsi_Host *shost)
>> +{
>> +    return context_unsafe(READ_ONCE(shost->shost_state));
> 
> I am wondering if it may be better to protect reading this with the 
> spinlock as well. We could lose the READ_ONCE and WRITE_ONCE. And we 
> would be more symmetrical with the set function.
> 
> I really don't feel strongly about this, though.

I slightly prefer READ_ONCE() because READ_ONCE() makes it clear that a
race condition is triggered. Protecting the host state read with a
spinlock is misleading in my opinion because it suppresses data race
reports while the host state can change as soon as the spinlock has
been unlocked.

>> +int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state 
>> state)
>> +    __must_hold(shost->host_lock);
> 
> How come we have this in the prototype and not the actual function itself?

Adding __must_hold() to the function declaration is sufficient. Adding
__must_hold() to the function definition is optional if it has already 
been added to the function definition.

Thanks,

Bart.