Re: [PATCH 09/12] iwlwifi: pcie: remove non-responsive device

[Kalle Valo <kvalo@codeaurora.org>]

Luca Coelho <luca@coelho.fi> writes:

> On Tue, 2018-04-24 at 12:44 +0300, Kalle Valo wrote:
>> Luca Coelho <luca@coelho.fi> writes:
>> 
>> > From: Luca Coelho <luciano.coelho@intel.com>
>> > 
>> > If we fail to to grab NIC access because the device is not
>> > responding
>> > (i.e. CSR_GP_CNTRL returns 0xFFFFFFFF), remove the device from the
>> > PCI
>> > bus, to avoid any further damage, and to let the user space rescan.
>> > 
>> > Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
>> > Signed-off-by: Rajat Jain <rajatja@google.com>
>> 
>> The commit log doesn't mention anything about the module parameter
>> nor
>> about the kobject uevent.
>
> Good point.  The uevent and the module parameter were added in later
> patches and I squashed them all into this one, but forgot to update the
> commit message.  I'll fix it.

Good, thanks.

>> > +static void iwl_trans_pcie_removal_wk(struct work_struct *wk)
>> > +{
>> > +	struct iwl_trans_pcie_removal *removal =
>> > +		container_of(wk, struct iwl_trans_pcie_removal,
>> > work);
>> > +	struct pci_dev *pdev = removal->pdev;
>> > +	char *prop[] = {"EVENT=INACCESSIBLE", NULL};
>> > +
>> > +	dev_err(&pdev->dev, "Device gone - attempting removal\n");
>> > +	kobject_uevent_env(&pdev->dev.kobj, KOBJ_CHANGE, prop);
>> > +	pci_lock_rescan_remove();
>> > +	pci_dev_put(pdev);
>> > +	pci_stop_and_remove_bus_device(pdev);
>> > +	pci_unlock_rescan_remove();
>> > +
>> > +	kfree(removal);
>> > +	module_put(THIS_MODULE);
>> > +}
>> 
>> So is the uevent some standard thing or what? At least grepping
>> INACCESSIBLE for didn't tell me anything.
>
> No, this is not standard.  We didn't find any appropriate standard
> message for this case, so we created a new one.  Also, we didn't want
> to interfere with components that may react to standard messages.
>
> This is disabled by default and the idea is to change this in the
> future to allow the driver to remove and rescan the device
> automatically.  So we will have 3 options in the module parameter: do
> nothing; auto-rescan or; send uevent.

Ok, I assume you will explain this in the commit log.

In my opinion the driver should not be sending custom events like this
and instead pci_stop_and_remove_bus_device() should do it, but maybe
that's just me?

-- 
Kalle Valo