Re: [PATCH 05/20] scsi: pm8001: Remove local variable in pm8001_pci_resume()

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2/10/22 21:04, John Garry wrote:
> On 10/02/2022 11:42, Damien Le Moal wrote:
>> In pm8001_pci_resume(), the use of the u32 type for the local variable
>> device_state causes a sparse warning:
>>
>> warning: incorrect type in assignment (different base types)
>>      expected unsigned int [usertype] device_state
>>      got restricted pci_power_t [usertype] current_state
>>
>> Since this variable is used only once in the function, remove it and
>> use pdev->current_state directly. While at it, also add a blank line
>> after the last local variable declaration.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> 
> Regardless of a couple of comments:
> Reviewed-by: John Garry <john.garry@huawei.com>
> 
>> ---
>>   drivers/scsi/pm8001/pm8001_init.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
>> index d8a2121cb8d9..4b9a26f008a9 100644
>> --- a/drivers/scsi/pm8001/pm8001_init.c
>> +++ b/drivers/scsi/pm8001/pm8001_init.c
>> @@ -1335,13 +1335,13 @@ static int __maybe_unused pm8001_pci_resume(struct device *dev)
>>   	struct pm8001_hba_info *pm8001_ha;
>>   	int rc;
>>   	u8 i = 0, j;
>> -	u32 device_state;
>>   	DECLARE_COMPLETION_ONSTACK(completion);
>> +
>>   	pm8001_ha = sha->lldd_ha;
>> -	device_state = pdev->current_state;
>>   
>> -	pm8001_info(pm8001_ha, "pdev=0x%p, slot=%s, resuming from previous operating state [D%d]\n",
>> -		      pdev, pm8001_ha->name, device_state);
>> +	pm8001_info(pm8001_ha,
>> +		    "pdev=0x%p, slot=%s, resuming from previous operating state [D%d]\n",
> 
> I think that we may put this on the same line as pm8001_info
> 
> Feel free to ignore this: if we're ok with changing logs, I am not sure 
> on the "slot" value - it is already printed with pm8001_info. And 
> printing pdev is suspect, since we should really be using dev_info or 
> pci_info() and friends - but that is a bigger job.

Yeah... This driver debug messages are a nightmare: hard to read format
and not-so-useful information. Not to mention that the default log level
is way too verbose... We should revisit the messages once we have
flushed out all know bugs :)

Thanks for the review.


> 
> Thanks,
> John
> 
>> +		    pdev, pm8001_ha->name, pdev->current_state);
>>   
>>   	rc = pci_go_44(pdev);
>>   	if (rc)
> 


-- 
Damien Le Moal
Western Digital Research