Re: [PATCH v3 2/3] scsi: core: Introduce a new list for SCSI proc directory entries

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

On 9/13/22 07:26, John Garry wrote:
> On 09/09/2022 00:35, Bart Van Assche wrote:
>> +struct scsi_proc_entry {
>> +    struct list_head    entry;
>> +    const struct scsi_host_template *sht;
>> +    struct proc_dir_entry    *proc_dir;
>> +    unsigned char        present;
> 
> is there really a hard limit of 255 hosts?

Apparently that limit exists today. I can use a wider data type for the 
'present' member if you want.

>> +static struct scsi_proc_entry *
>> +__scsi_lookup_proc_entry(const struct scsi_host_template *sht)
>> +{
>> +    struct scsi_proc_entry *e;
>> +
>> +    lockdep_assert_held(&global_host_template_mutex);
> 
> I'm not sure we really need this - maybe a comment would be better. I 
> don't care too much either way.

lockdep_assert_held() statements get verified at runtime if lockdep is 
enabled but comments not. This is why I prefer lockdep_assert_held() 
over a comment.

>> +    if (e->present++) {
>> +        e = NULL;
>> +        goto unlock;
>> +    }
>> +    e->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
>> +    if (!e->proc_dir) {
>> +        printk(KERN_ERR "%s: proc_mkdir failed for %s\n", __func__,
>> +               sht->proc_name);
>> +        goto unlock;
> 
> hmmm... in this case we don't add e to the list. As unlikely as it 
> seems, if a subsequent attempt to create the proc dir passes for another 
> host with the same sht, then e->present would be incorrect, right?

Right, this needs to be fixed.

Thanks,

Bart.