| Message ID | 1348080894-23412-2-git-send-email-yinghai@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Sep 19, 2012 at 12:54 PM, Yinghai Lu <yinghai@kernel.org> wrote: > when __ARCH_HAS_VGA_DEFAULT_DEVICE is not defined, aka EFIFB is not used, > for static path, vga_default setting is through vga_arbiter_add_pci_device. > and later x86 pci_fixup_video, will skip setting again. > - subsys_initcall(vga_arb_device_init) come first to call vga_arbiter_add_pci_device. It will call pci_get_dev to hold one reference. > > for hotplug add path, even vga_arbiter_add_pci_device is called via notifier, > but it will check VGA_RSRC_LEGACY_MASK that is not set for hotplug path. > So x86 pci_fixup_video will take over to call vga_set_default_device(). > It will not hold one refrence. > > Later for hotplug remove path, vga_arbiter_del_pci_device that does not check > VGA_RSRC_LEGACY_MASK will call put_device and it will cause ref_count to > decrease extra. that will have that pci device get deleted early wrongly. > > Need to make get/put balance for both cases. > > -v2: According to Bjorn, update vga_set_default_device instead... > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Cc: x86@kernel.org > Cc: Dave Airlie <airlied@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Julia Lawall <julia@diku.dk> > Cc: Matthew Garrett <mjg@redhat.com> > --- > drivers/gpu/vga/vgaarb.c | 14 +++++++++----- > 1 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c > index b6852b7..0888951e 100644 > --- a/drivers/gpu/vga/vgaarb.c > +++ b/drivers/gpu/vga/vgaarb.c > @@ -141,6 +141,12 @@ EXPORT_SYMBOL_GPL(vga_default_device); > > void vga_set_default_device(struct pci_dev *pdev) > { > + if (vga_default) > + pci_dev_put(vga_default); > + > + if (pdev) > + pdev = pci_dev_get(pdev); > + > vga_default = pdev; I think this is equivalent to: pci_dev_put(vga_default); vga_default = pci_dev_get(pdev); I haven't seen an answer to Matthew's question about whether we want to check for "vga_default == pdev". > } > #endif > @@ -577,7 +583,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) > #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE > if (vga_default == NULL && > ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) > - vga_default = pci_dev_get(pdev); > + vga_set_default_device(pdev); > #endif > > vga_arbiter_check_bridge_sharing(vgadev); > @@ -613,10 +619,8 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev) > } > > #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE > - if (vga_default == pdev) { > - pci_dev_put(vga_default); > - vga_default = NULL; > - } > + if (vga_default == pdev) > + vga_set_default_device(NULL); > #endif > > if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM)) > -- > 1.7.7 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 21, 2012 at 1:52 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Wed, Sep 19, 2012 at 12:54 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> --- a/drivers/gpu/vga/vgaarb.c >> +++ b/drivers/gpu/vga/vgaarb.c >> @@ -141,6 +141,12 @@ EXPORT_SYMBOL_GPL(vga_default_device); >> >> void vga_set_default_device(struct pci_dev *pdev) >> { >> + if (vga_default) >> + pci_dev_put(vga_default); >> + >> + if (pdev) >> + pdev = pci_dev_get(pdev); >> + >> vga_default = pdev; > > I think this is equivalent to: > > pci_dev_put(vga_default); > vga_default = pci_dev_get(pdev); ah, i missed that, pci_dev_put and pci_dev_get check that inside. > > I haven't seen an answer to Matthew's question about whether we want > to check for "vga_default == pdev". yes, that could avoid the extra put/get pair. Please check updated version. -Yinghai
On 09/20/2012 02:54 AM, Yinghai Lu wrote: > when __ARCH_HAS_VGA_DEFAULT_DEVICE is not defined, aka EFIFB is not used, > for static path, vga_default setting is through vga_arbiter_add_pci_device. > and later x86 pci_fixup_video, will skip setting again. > - subsys_initcall(vga_arb_device_init) come first to call vga_arbiter_add_pci_device. It will call pci_get_dev to hold one reference. > > for hotplug add path, even vga_arbiter_add_pci_device is called via notifier, > but it will check VGA_RSRC_LEGACY_MASK that is not set for hotplug path. > So x86 pci_fixup_video will take over to call vga_set_default_device(). > It will not hold one refrence. > > Later for hotplug remove path, vga_arbiter_del_pci_device that does not check > VGA_RSRC_LEGACY_MASK will call put_device and it will cause ref_count to > decrease extra. that will have that pci device get deleted early wrongly. > > Need to make get/put balance for both cases. > > -v2: According to Bjorn, update vga_set_default_device instead... > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Cc: x86@kernel.org > Cc: Dave Airlie <airlied@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Julia Lawall <julia@diku.dk> > Cc: Matthew Garrett <mjg@redhat.com> > --- > drivers/gpu/vga/vgaarb.c | 14 +++++++++----- > 1 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c > index b6852b7..0888951e 100644 > --- a/drivers/gpu/vga/vgaarb.c > +++ b/drivers/gpu/vga/vgaarb.c > @@ -141,6 +141,12 @@ EXPORT_SYMBOL_GPL(vga_default_device); > > void vga_set_default_device(struct pci_dev *pdev) > { > + if (vga_default) > + pci_dev_put(vga_default); > + > + if (pdev) > + pdev = pci_dev_get(pdev); > + > vga_default = pdev; > } Seems it could be simplified as void vga_set_default_device(struct pci_dev *pdev) { pci_dev_put(vga_default); vga_default = pci_dev_get(pdev); } --Gerry > #endif > @@ -577,7 +583,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) > #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE > if (vga_default == NULL && > ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) > - vga_default = pci_dev_get(pdev); > + vga_set_default_device(pdev); > #endif > > vga_arbiter_check_bridge_sharing(vgadev); > @@ -613,10 +619,8 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev) > } > > #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE > - if (vga_default == pdev) { > - pci_dev_put(vga_default); > - vga_default = NULL; > - } > + if (vga_default == pdev) > + vga_set_default_device(NULL); > #endif > > if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM)) > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index b6852b7..0888951e 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/vgaarb.c @@ -141,6 +141,12 @@ EXPORT_SYMBOL_GPL(vga_default_device); void vga_set_default_device(struct pci_dev *pdev) { + if (vga_default) + pci_dev_put(vga_default); + + if (pdev) + pdev = pci_dev_get(pdev); + vga_default = pdev; } #endif @@ -577,7 +583,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev) #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE if (vga_default == NULL && ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK)) - vga_default = pci_dev_get(pdev); + vga_set_default_device(pdev); #endif vga_arbiter_check_bridge_sharing(vgadev); @@ -613,10 +619,8 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev) } #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE - if (vga_default == pdev) { - pci_dev_put(vga_default); - vga_default = NULL; - } + if (vga_default == pdev) + vga_set_default_device(NULL); #endif if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
when __ARCH_HAS_VGA_DEFAULT_DEVICE is not defined, aka EFIFB is not used, for static path, vga_default setting is through vga_arbiter_add_pci_device. and later x86 pci_fixup_video, will skip setting again. - subsys_initcall(vga_arb_device_init) come first to call vga_arbiter_add_pci_device. It will call pci_get_dev to hold one reference. for hotplug add path, even vga_arbiter_add_pci_device is called via notifier, but it will check VGA_RSRC_LEGACY_MASK that is not set for hotplug path. So x86 pci_fixup_video will take over to call vga_set_default_device(). It will not hold one refrence. Later for hotplug remove path, vga_arbiter_del_pci_device that does not check VGA_RSRC_LEGACY_MASK will call put_device and it will cause ref_count to decrease extra. that will have that pci device get deleted early wrongly. Need to make get/put balance for both cases. -v2: According to Bjorn, update vga_set_default_device instead... Signed-off-by: Yinghai Lu <yinghai@kernel.org> Cc: x86@kernel.org Cc: Dave Airlie <airlied@redhat.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Julia Lawall <julia@diku.dk> Cc: Matthew Garrett <mjg@redhat.com> --- drivers/gpu/vga/vgaarb.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-)