On Wed, 2018-04-25 at 11:01 +0300, Kalle Valo wrote:
> Luca Coelho <luca@xxxxxxxxx> writes:
>
> > On Tue, 2018-04-24 at 12:44 +0300, Kalle Valo wrote:
> > > Luca Coelho <luca@xxxxxxxxx> writes:
> > >
> > > > From: Luca Coelho <luciano.coelho@xxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > > > Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx>
> > >
> > > 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.
Yes, I'll improve the commit message.
> 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?
pci_stop_and_remove_bus_device() will already trigger some events, like
"REMOVED" or something like that. But the semantics is a little bit
different. If the device was removed, you don't usually want to rescan
the bus, because the device will not be there anymore. In our case,
the device *is* there, but got stuck and we want to rescan to get it
back.
--
Cheers,
Luca.
