Re: [PATCH 02/21] lpfc: Add Lancer Temperature Event support to the lpfc driver

[James Smart <james.smart@emulex.com>]

On 2/6/2015 7:16 AM, Tomas Henzl wrote:
> On 02/05/2015 08:23 PM, James Smart wrote:
>> ---
>>   drivers/scsi/lpfc/lpfc_hw4.h  |   1 +
>>   drivers/scsi/lpfc/lpfc_init.c | 179 +++++++++++++++++++++++++++++-------------
>>   2 files changed, 125 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/scsi/lpfc/lpfc_hw4.h b/drivers/scsi/lpfc/lpfc_hw4.h
>> index f432ec1..3121ec4 100644
>> --- a/drivers/scsi/lpfc/lpfc_hw4.h
>> +++ b/drivers/scsi/lpfc/lpfc_hw4.h
>> @@ -3244,6 +3244,7 @@ struct lpfc_acqe_sli {
>>   #define LPFC_SLI_EVENT_TYPE_NVLOG_POST		0x4
>>   #define LPFC_SLI_EVENT_TYPE_DIAG_DUMP		0x5
>>   #define LPFC_SLI_EVENT_TYPE_MISCONFIGURED	0x9
>> +#define LPFC_SLI_EVENT_TYPE_REMOTE_DPORT	0xA
>>   };
>>   
>>   /*
>> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
>> index 2b5b910..4ba91af 100644
>> --- a/drivers/scsi/lpfc/lpfc_init.c
>> +++ b/drivers/scsi/lpfc/lpfc_init.c
>> @@ -1330,13 +1330,14 @@ lpfc_offline_eratt(struct lpfc_hba *phba)
>>   void
>>   lpfc_sli4_offline_eratt(struct lpfc_hba *phba)
>>   {
>> +	spin_lock_irq(&phba->hbalock);
>> +	phba->link_state = LPFC_HBA_ERROR;
>> +	spin_unlock_irq(&phba->hbalock);
> Hi James,
> please explain why is the spinlock^ needed?
> There seems to be lot of other places where link_state is not protected,
> for example it is evaluated in lpfc_sli4_handle_received_buffer with no lock.
>
> Could you please also add some more description to the of the body your mails,
> sometimes  a subject only is not enough.
>
> Thanks,
> Tomas

Well - good question - access should be under lock, and it wouldn't 
surprise me if there are some cases, especially read checks, where it 
isn't. In most cases, this bad behavior works, as the variable doesn't 
change often, and even when it does, there's very long time delays 
before something would reference the change. Not trying to say it 
shouldn't be fixed, but rather why it hasn't been an issue. This one 
will take a little bit to clean up - it reached this point historically 
over many successive changes.

Yes, I'll add a little meat to future patches.

-- james s