Re: [PATCH] scsi: ses: don't get power status of SES device slot on probe

[Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>]

Hi Dan,

Thanks for reviewing.

On 04/04/2017 06:07 PM, Dan Williams wrote:
>> @@ -594,6 +594,10 @@ static ssize_t get_component_power_status(struct device *cdev,
>>
>>         if (edev->cb->get_power_status)
>>                 edev->cb->get_power_status(edev, ecomp);
>> +
>> +       if (ecomp->power_status == -1)
>> +               return -EINVAL;
>> +
> Can we ever hit this if get_power_status is non-null?

I admit that is cautionary, and more for consistency with the chance
of an uninitialized state being still in place, so not to return any
(incorrect) value in that state.

But, yes, in a few cases -- although unlikely.

1) imagine .get_power_status couldn't update the 'power_status' field
    (it's a bit unlikely with the in-tree ses driver, but in the case
     that ses_get_page2_descriptor() returns NULL, ses_get_power_status()
     doesn't update 'power_status').

2) an out-of-tree module employs another enclosure_component_callbacks
    structure, which doesn't implement a .get_power_status hook.

3) or it does, but that failed, and didn't update 'power_status' (e.g.,
    like case 1)


>> +static bool power_status_on_probe;     /* default: false, 0. */
>> +module_param(power_status_on_probe, bool, 0644);
>> +MODULE_PARM_DESC(power_status_on_probe, "get power status of SES device slots "
>> +                                       "(enclosure components) at probe time "
>> +                                       "(warning: may delay total probe time)");
>> +
> I don't think we need this module parameter as long as the only way to
> read the power status is through sysfs. We can always just leave it to
> be read on-demand when needed.

I agree for the in-tree module.  My only concern, as described in the
commit message, is the case someone is using an out-of-tree module /
has an implementation that depends on that (e.g., reading the 'power_
status' field directly, instead of using .get_power_status.

I've been a bit overzealous when considering why that call was inserted
in ses_enclosure_data_process(), and didn't want to break them.

However, for an in-tree usage, it's clear that just dropping that line
seemed to be sufficient -- the rest in the patch is just consideration
for whoever might be using it in out-of-tree ways.

(and if such consideration is deemed not to be required in this case,
  I'd be fine with dropping the related changes and making this patch
  a one-line. :- )

cheers,

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center