On Tue, Dec 13, 2016 at 08:50:07PM -0600, Bjorn Helgaas wrote:
> [+cc Alan, Greg]
>
> On Tue, Dec 13, 2016 at 06:07:31PM -0500, Keith Busch wrote:
> > On Tue, Dec 13, 2016 at 02:56:14PM -0600, Bjorn Helgaas wrote:
> > > On Fri, Oct 28, 2016 at 06:58:15PM -0400, Keith Busch wrote:
> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > index 0e49f70..2115d19 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -341,6 +341,7 @@ struct pci_dev {
> > > > unsigned int multifunction:1;/* Part of multi-function device */
> > > > /* keep track of device state */
> > > > unsigned int is_added:1;
> > > > + unsigned int is_removed:1; /* device was surprise removed */
> > > > unsigned int is_busmaster:1; /* device is busmaster */
> > > > unsigned int no_msi:1; /* device may not use msi */
> > > > unsigned int no_64bit_msi:1; /* device may only use 32-bit MSIs */
> > > > @@ -417,6 +418,12 @@ static inline int pci_channel_offline(struct pci_dev *pdev)
> > > > return (pdev->error_state != pci_channel_io_normal);
> > > > }
> > > >
> > > > +static inline int pci_set_removed(struct pci_dev *pdev, void *unused)
> > > > +{
> > > > + pdev->is_removed = 1;
> > >
> > > This makes me slightly worried because this is a bitfield and there's
> > > no locking. A concurrent write to some nearby field can corrupt
> > > things. It doesn't look *likely*, but it's a lot of work to be
> > > convinced that this is completely safe, especially since the writer is
> > > running on behalf of the bridge, and the target is a child of the
> > > bridge.
> > >
> > > The USB HCD_FLAG_DEAD and HCD_FLAG_HW_ACCESSIBLE flags are somewhat
> > > similar. Maybe we can leverage some of that design?
> >
> > A bit field with atomic accessors sounds good to me. Do you want to
> > see all the struct pci_dev bit flags converted to that model? If so,
> > I can send you a prep patch that does that first.
>
> This is still blue-sky, "what if?" thinking on my part. I was
> starting to wonder if we could make something generic that could be
> used for your is_removed work and also for the USB stuff.
>
> The USB HCD_FLAG_DEAD is set by usb_hc_died(), which is frequently
> used by drivers after an MMIO read returns ~0. HCD_FLAG_HW_ACCESSIBLE
> seems to encode part of the idea of "this device is powered up enough
> to respond", which is sort of similar to what PCI does with
> dev->current_state.
>
> If there were generic PCI interfaces like pci_dev_died() and a
> pci_dev_dead() predicate, I wonder if USB could use them instead of
> rolling their own?
But I guess all the world is not PCI -- there are non-PCI USB host
controllers, so it's not quite that simple.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
