Patchwork PCI/portdrv: Set master earlier

login
register
mail settings
Submitter Jean Delvare
Date Oct. 9, 2013, 2:33 p.m.
Message ID <20131009163304.62b439a9@endymion.delvare>
Download mbox | patch
Permalink /patch/281906/
State Not Applicable
Headers show

Comments

Jean Delvare - Oct. 9, 2013, 2:33 p.m.
Since kernel 3.12-rc3, I get the following warning messages at boot:
pcieport 0000:00:07.0: driver skip pci_set_master, fix it!
pcieport 0000:00:01.0: driver skip pci_set_master, fix it!

These are:
00:01.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root Port 1 (rev 13)
00:07.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root Port 7 (rev 13)

Calling pci_set_master() immediately after pci_enable_device() makes
the warning messages go away.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
Please note that I am far from certain that this is the correct fix.
Another approach would be to disable the device if
get_port_device_capability() returns no capabilities. I suppose the
device would then be re-enabled later if really needed. What's the best
approach?

 drivers/pci/pcie/portdrv_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Bjorn Helgaas - Dec. 7, 2013, 10:23 p.m.
[+cc Yinghai]

On Wed, Oct 9, 2013 at 8:33 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Since kernel 3.12-rc3, I get the following warning messages at boot:
> pcieport 0000:00:07.0: driver skip pci_set_master, fix it!
> pcieport 0000:00:01.0: driver skip pci_set_master, fix it!

cf3e1feba7f9 ("PCI: Workaround missing pci_set_master in pci drivers")
added this warning and, at the same time, turned on bus mastering.

There didn't seem to be any reason for the warning and no reason why
bus mastering should be enabled in the driver rather than the core, so
fbeeb822f6f4 ("PCI: Drop warning about drivers that don't use
pci_set_master()") just dropped the warning.

This should resolve the problem, so I don't think there's a need for
this patch.  Let me know if otherwise.

Bjorn

> These are:
> 00:01.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root Port 1 (rev 13)
> 00:07.0 PCI bridge: Intel Corporation 5520/5500/X58 I/O Hub PCI Express Root Port 7 (rev 13)
>
> Calling pci_set_master() immediately after pci_enable_device() makes
> the warning messages go away.
>
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> ---
> Please note that I am far from certain that this is the correct fix.
> Another approach would be to disable the device if
> get_port_device_capability() returns no capabilities. I suppose the
> device would then be re-enabled later if really needed. What's the best
> approach?
>
>  drivers/pci/pcie/portdrv_core.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- linux-3.12-rc4.orig/drivers/pci/pcie/portdrv_core.c 2013-09-24 00:41:09.000000000 +0200
> +++ linux-3.12-rc4/drivers/pci/pcie/portdrv_core.c      2013-10-09 15:43:54.205943783 +0200
> @@ -366,13 +366,13 @@ int pcie_port_device_register(struct pci
>         status = pci_enable_device(dev);
>         if (status)
>                 return status;
> +       pci_set_master(dev);
>
>         /* Get and check PCI Express port services */
>         capabilities = get_port_device_capability(dev);
>         if (!capabilities)
>                 return 0;
>
> -       pci_set_master(dev);
>         /*
>          * Initialize service irqs. Don't use service devices that
>          * require interrupts if there is no way to generate them.
>
>
> --
> Jean Delvare
> Suse L3 Support
--
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
Yinghai Lu - Dec. 8, 2013, 3:26 a.m.
[+to BenH]

On Sat, Dec 7, 2013 at 2:23 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Yinghai]
>
> On Wed, Oct 9, 2013 at 8:33 AM, Jean Delvare <jdelvare@suse.de> wrote:
>> Since kernel 3.12-rc3, I get the following warning messages at boot:
>> pcieport 0000:00:07.0: driver skip pci_set_master, fix it!
>> pcieport 0000:00:01.0: driver skip pci_set_master, fix it!
>
> cf3e1feba7f9 ("PCI: Workaround missing pci_set_master in pci drivers")
> added this warning and, at the same time, turned on bus mastering.
>
> There didn't seem to be any reason for the warning and no reason why
> bus mastering should be enabled in the driver rather than the core, so
> fbeeb822f6f4 ("PCI: Drop warning about drivers that don't use
> pci_set_master()") just dropped the warning.
>
> This should resolve the problem, so I don't think there's a need for
> this patch.  Let me know if otherwise.

BenH mentioned that get_port_device_capability have some problem on powerpc
and will return null.

>>
>>  drivers/pci/pcie/portdrv_core.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> --- linux-3.12-rc4.orig/drivers/pci/pcie/portdrv_core.c 2013-09-24 00:41:09.000000000 +0200
>> +++ linux-3.12-rc4/drivers/pci/pcie/portdrv_core.c      2013-10-09 15:43:54.205943783 +0200
>> @@ -366,13 +366,13 @@ int pcie_port_device_register(struct pci
>>         status = pci_enable_device(dev);
>>         if (status)
>>                 return status;
>> +       pci_set_master(dev);
>>
>>         /* Get and check PCI Express port services */
>>         capabilities = get_port_device_capability(dev);
>>         if (!capabilities)
>>                 return 0;
>>
>> -       pci_set_master(dev);
>>         /*
>>          * Initialize service irqs. Don't use service devices that
>>          * require interrupts if there is no way to generate them.
>>
>>
--
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
Jean Delvare - Dec. 8, 2013, 1:09 p.m.
Hi Bjorn,

On Sat, 7 Dec 2013 15:23:25 -0700, Bjorn Helgaas wrote:
> [+cc Yinghai]
> 
> On Wed, Oct 9, 2013 at 8:33 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > Since kernel 3.12-rc3, I get the following warning messages at boot:
> > pcieport 0000:00:07.0: driver skip pci_set_master, fix it!
> > pcieport 0000:00:01.0: driver skip pci_set_master, fix it!
> 
> cf3e1feba7f9 ("PCI: Workaround missing pci_set_master in pci drivers")
> added this warning and, at the same time, turned on bus mastering.
> 
> There didn't seem to be any reason for the warning and no reason why
> bus mastering should be enabled in the driver rather than the core, so
> fbeeb822f6f4 ("PCI: Drop warning about drivers that don't use
> pci_set_master()") just dropped the warning.
> 
> This should resolve the problem, so I don't think there's a need for
> this patch.  Let me know if otherwise.

This is fine with me, all I really wanted was to get rid of the
warning. I am not aware of any problem either (but I am no PCI expert
by any means.)

Thanks,
Benjamin Herrenschmidt - Dec. 9, 2013, 3:07 a.m.
On Sat, 2013-12-07 at 19:26 -0800, Yinghai Lu wrote:
> BenH mentioned that get_port_device_capability have some problem on
> powerpc and will return null.

Well, it has problems on anything that doesn't have ACPI :-)

I don't care what the "right" fix is but we need to makes sure nobody
does a pci_enable_device() on a bridge and doesn't also enable bus
master, otherwise the bridge bus master will never be set. This is what
was happening an breaking us.

Ben.


--
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
Bjorn Helgaas - Dec. 17, 2013, 12:53 a.m.
On Sun, Dec 8, 2013 at 8:07 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Sat, 2013-12-07 at 19:26 -0800, Yinghai Lu wrote:
>> BenH mentioned that get_port_device_capability have some problem on
>> powerpc and will return null.
>
> Well, it has problems on anything that doesn't have ACPI :-)
>
> I don't care what the "right" fix is but we need to makes sure nobody
> does a pci_enable_device() on a bridge and doesn't also enable bus
> master, otherwise the bridge bus master will never be set. This is what
> was happening an breaking us.

I assume there's nothing more I need to do here.  If there is, please
let me know.

Bjorn
--
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

Patch

--- linux-3.12-rc4.orig/drivers/pci/pcie/portdrv_core.c	2013-09-24 00:41:09.000000000 +0200
+++ linux-3.12-rc4/drivers/pci/pcie/portdrv_core.c	2013-10-09 15:43:54.205943783 +0200
@@ -366,13 +366,13 @@  int pcie_port_device_register(struct pci
 	status = pci_enable_device(dev);
 	if (status)
 		return status;
+	pci_set_master(dev);
 
 	/* Get and check PCI Express port services */
 	capabilities = get_port_device_capability(dev);
 	if (!capabilities)
 		return 0;
 
-	pci_set_master(dev);
 	/*
 	 * Initialize service irqs. Don't use service devices that
 	 * require interrupts if there is no way to generate them.