Re: [PATCH] lpfc: Avoid to disable pci_dev twice

[Mike Qiu <qiudayu@linux.vnet.ibm.com>]

On 08/28/2014 02:34 AM, James Smart wrote:
> Mike,
>
> Can you confirm - the "nulls" this patch correct are because the 
> probe_one and error_detect threads are running concurrently, thus 
> battling ?
>
> If so - this fix looks insufficient and we should rework it.

Yes, it is. My patch is just a workaround for this bug.

>
> Q: why are they allowed to run concurrently ?  I could see this solved 
> at the platform level to let probe_one finish before error_detect is 
> called (and therefore stating error_detect only makes sense to call if 
> probe_one was successful). It's also a much driver-friendly solution. 
> I could see other drivers have much the same issue with concurrency 
> and data structure teardown - and if locks aren't allowed in the 
> error-detect path... it's not good.
>

I agree with you on this point, platform solution is much better. So 
maybe use a lock or a flag to show it is in such stat, this maybe  also 
happens when driver is in  remove stat.

Thanks,
Mike
> -- james s
>
>
>
> On 7/31/2014 10:16 PM, Mike Qiu wrote:
>> On 07/17/2014 02:32 PM, Mike Qiu wrote:
>>
>>
>> Hi, all
>>
>> How about this patch ?
>>
>> Any idea ?
>>
>>> In IBM Power servers, when hardware error occurs during probe
>>> state, EEH subsystem will call driver's error_detected interface,
>>> which will call pci_disable_device(). But driver's probe function also
>>> call pci_disable_device() in this situation.
>>>
>>> So pci_dev will be disabled twice:
>>>
>>> Device lpfc disabling already-disabled device
>>> ------------[ cut here ]------------
>>> WARNING: at drivers/pci/pci.c:1407
>>> CPU: 0 PID: 8744 Comm: kworker/0:0 Tainted: G        W 
>>> 3.10.42-2002.pkvm2_1_1.6.ppc64 #1
>>> Workqueue: events .work_for_cpu_fn
>>> task: c00000274e3f5400 ti: c0000027d3958000 task.ti: c0000027d3958000
>>> NIP: c000000000471b8c LR: c000000000471b88 CTR: c00000000043ebe0
>>> REGS: c0000027d395b650 TRAP: 0700   Tainted: G        W 
>>> (3.10.42-2002.pkvm2_1_1.6.ppc64)
>>> MSR: 9000000100029032 <SF,HV,EE,ME,IR,DR,RI>  CR: 28b52b44  XER: 
>>> 20000000
>>> CFAR: c000000000879ab8 SOFTE: 1
>>> ...
>>> NIP .pci_disable_device+0xcc/0xe0
>>> LR  .pci_disable_device+0xc8/0xe0
>>> Call Trace:
>>> .pci_disable_device+0xc8/0xe0 (unreliable)
>>> .lpfc_disable_pci_dev+0x50/0x80 [lpfc]
>>> .lpfc_pci_probe_one+0x870/0x21a0 [lpfc]
>>> .local_pci_probe+0x68/0xb0
>>> .work_for_cpu_fn+0x38/0x60
>>> .process_one_work+0x1a4/0x4d0
>>> .worker_thread+0x37c/0x490
>>> .kthread+0xf0/0x100
>>> .ret_from_kernel_thread+0x5c/0x80
>>>
>>> Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
>>> ---
>>>   drivers/scsi/lpfc/lpfc.h      |  1 +
>>>   drivers/scsi/lpfc/lpfc_init.c | 59 
>>> +++++++++++++++++++++++++++++++++++++++----
>>>   2 files changed, 55 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/scsi/lpfc/lpfc.h b/drivers/scsi/lpfc/lpfc.h
>>> index 434e903..0c7bad9 100644
>>> --- a/drivers/scsi/lpfc/lpfc.h
>>> +++ b/drivers/scsi/lpfc/lpfc.h
>>> @@ -813,6 +813,7 @@ struct lpfc_hba {
>>>   #define VPD_MASK            0xf         /* mask for any vpd data */
>>>
>>>       uint8_t soft_wwn_enable;
>>> +    uint8_t probe_done;
>>>
>>>       struct timer_list fcp_poll_timer;
>>>       struct timer_list eratt_poll;
>>> diff --git a/drivers/scsi/lpfc/lpfc_init.c 
>>> b/drivers/scsi/lpfc/lpfc_init.c
>>> index 06f9a5b..c2e67ae 100644
>>> --- a/drivers/scsi/lpfc/lpfc_init.c
>>> +++ b/drivers/scsi/lpfc/lpfc_init.c
>>> @@ -9519,6 +9519,9 @@ lpfc_pci_probe_one_s3(struct pci_dev *pdev, 
>>> const struct pci_device_id *pid)
>>>           }
>>>       }
>>>
>>> +    /* Set the probe flag */
>>> +    phba->probe_done = 1;
>>> +
>>>       /* Perform post initialization setup */
>>>       lpfc_post_init_setup(phba);
>>>
>>> @@ -9795,6 +9798,9 @@ lpfc_sli_prep_dev_for_recover(struct lpfc_hba 
>>> *phba)
>>>   static void
>>>   lpfc_sli_prep_dev_for_reset(struct lpfc_hba *phba)
>>>   {
>>> +    if (phba)
>>> +        return;
>>> +
>>>       lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
>>>               "2710 PCI channel disable preparing for reset\n");
>>>
>>> @@ -9812,7 +9818,8 @@ lpfc_sli_prep_dev_for_reset(struct lpfc_hba 
>>> *phba)
>>>
>>>       /* Disable interrupt and pci device */
>>>       lpfc_sli_disable_intr(phba);
>>> -    pci_disable_device(phba->pcidev);
>>> +    if (phba->probe_done && phba->pcidev)
>>> +        pci_disable_device(phba->pcidev);
>>>   }
>>>
>>>   /**
>>> @@ -10282,6 +10289,9 @@ lpfc_pci_probe_one_s4(struct pci_dev *pdev, 
>>> const struct pci_device_id *pid)
>>>           goto out_disable_intr;
>>>       }
>>>
>>> +    /* Set probe_done flag */
>>> +    phba->probe_done = 1;
>>> +
>>>       /* Log the current active interrupt mode */
>>>       phba->intr_mode = intr_mode;
>>>       lpfc_log_intr_mode(phba, intr_mode);
>>> @@ -10544,6 +10554,9 @@ lpfc_sli4_prep_dev_for_recover(struct 
>>> lpfc_hba *phba)
>>>   static void
>>>   lpfc_sli4_prep_dev_for_reset(struct lpfc_hba *phba)
>>>   {
>>> +    if (!phba)
>>> +        return;
>>> +
>>>       lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
>>>               "2826 PCI channel disable preparing for reset\n");
>>>
>>> @@ -10562,7 +10575,9 @@ lpfc_sli4_prep_dev_for_reset(struct lpfc_hba 
>>> *phba)
>>>       /* Disable interrupt and pci device */
>>>       lpfc_sli4_disable_intr(phba);
>>>       lpfc_sli4_queue_destroy(phba);
>>> -    pci_disable_device(phba->pcidev);
>>> +
>>> +    if (phba->probe_done && phba->pcidev)
>>> +        pci_disable_device(phba->pcidev);
>>>   }
>>>
>>>   /**
>>> @@ -10893,9 +10908,21 @@ static pci_ers_result_t
>>>   lpfc_io_error_detected(struct pci_dev *pdev, pci_channel_state_t 
>>> state)
>>>   {
>>>       struct Scsi_Host *shost = pci_get_drvdata(pdev);
>>> -    struct lpfc_hba *phba = ((struct lpfc_vport 
>>> *)shost->hostdata)->phba;
>>> +    struct lpfc_hba *phba;
>>>       pci_ers_result_t rc = PCI_ERS_RESULT_DISCONNECT;
>>>
>>> +    if (!shost)
>>> +        /* Run here means it may during probe state and
>>> +         * Scsi_Host has not been created and We can do nothing
>>> +         * in this state so call for hotplug*/
>>> +        return PCI_ERS_RESULT_NONE;
>>> +
>>> +    phba = ((struct lpfc_vport *)shost->hostdata)->phba;
>>> +
>>> +    if (!phba || !phba->probe_done)
>>> +        /* Run here means it may during probe state */
>>> +        return PCI_ERS_RESULT_NONE;
>>> +
>>>       switch (phba->pci_dev_grp) {
>>>       case LPFC_PCI_DEV_LP:
>>>           rc = lpfc_io_error_detected_s3(pdev, state);
>>> @@ -10930,9 +10957,20 @@ static pci_ers_result_t
>>>   lpfc_io_slot_reset(struct pci_dev *pdev)
>>>   {
>>>       struct Scsi_Host *shost = pci_get_drvdata(pdev);
>>> -    struct lpfc_hba *phba = ((struct lpfc_vport 
>>> *)shost->hostdata)->phba;
>>> +    struct lpfc_hba *phba;
>>>       pci_ers_result_t rc = PCI_ERS_RESULT_DISCONNECT;
>>>
>>> +    if (!shost)
>>> +        /* Run here means it may during probe state and
>>> +         * Scsi_Host has not been created */
>>> +        return PCI_ERS_RESULT_NONE;
>>> +
>>> +    phba = ((struct lpfc_vport *)shost->hostdata)->phba;
>>> +
>>> +    if (!phba || !phba->probe_done)
>>> +        /* Run here means it may during probe state */
>>> +        return PCI_ERS_RESULT_NONE;
>>> +
>>>       switch (phba->pci_dev_grp) {
>>>       case LPFC_PCI_DEV_LP:
>>>           rc = lpfc_io_slot_reset_s3(pdev);
>>> @@ -10963,7 +11001,18 @@ static void
>>>   lpfc_io_resume(struct pci_dev *pdev)
>>>   {
>>>       struct Scsi_Host *shost = pci_get_drvdata(pdev);
>>> -    struct lpfc_hba *phba = ((struct lpfc_vport 
>>> *)shost->hostdata)->phba;
>>> +    struct lpfc_hba *phba;
>>> +
>>> +    if (!shost)
>>> +        /* Run here means it may during probe state and
>>> +         * Scsi_Host has not been created */
>>> +        return;
>>> +
>>> +    phba = ((struct lpfc_vport *)shost->hostdata)->phba;
>>> +
>>> +    if (!phba || !phba->probe_done)
>>> +        /* Run here means it may during probe state */
>>> +        return;
>>>
>>>       switch (phba->pci_dev_grp) {
>>>       case LPFC_PCI_DEV_LP:
>>
>>
>