Re: [PATCH v6 0/5] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Wed, Oct 21, 2015 at 10:14:59AM -0500, Bjorn Helgaas wrote:
> 
> Applied to pci/enhanced-allocation for v4.4, thanks, David!
> 
> I tweaked a couple trivial things and added a couple almost trivial patches
> on top.  Here are the additions; let me know if you see anything wrong.
> 
> Bjorn
> 

Thanks Bjorn, 

I tested out your additions. They worked for me.

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 0/5] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Wed, Oct 21, 2015 at 10:14:59AM -0500, Bjorn Helgaas wrote:
> 
> Applied to pci/enhanced-allocation for v4.4, thanks, David!
> 
> I tweaked a couple trivial things and added a couple almost trivial patches
> on top.  Here are the additions; let me know if you see anything wrong.
> 
> Bjorn
> 

Thanks Bjorn, 

I tested out your additions. They worked for me.

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 0/4] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

Signed-off-by: Sean O. Stalley 

I tested it out with the QEMU EA Patches here:
[https://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg00348.html]

Also, I found 1 trivial typo in the commit message of PATCH 1/4:
"Signed-off-by: Signed-off-by: David Daney "

-Sean

On Wed, Oct 07, 2015 at 06:44:52AM -0700, Stalley, Sean wrote:
> [PATCH 3/4 & 4/4] Acked-by: Sean O. Stalley 
> 
> I won't be able to test it out until next week, but I like how it looks :)
> 
> Thanks Again,
> Sean
> 
> > -Original Message-
> > From: David Daney [mailto:ddaney.c...@gmail.com]
> > Sent: Tuesday, October 06, 2015 4:51 PM
> > To: linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; Bjorn Helgaas;
> > Michael S. Tsirkin; Rafał Miłecki; linux-...@vger.kernel.org; Stalley, Sean;
> > ying...@kernel.org; rajatxj...@gmail.com; gong.c...@linux.intel.com
> > Cc: David Daney
> > Subject: [PATCH v5 0/4] PCI: Add support for PCI Enhanced Allocation
> > "BARs"
> > 
> > From: David Daney 
> > 
> > The original patches are from Sean O. Stalley. I made a few tweaks, but feel
> > that it is substancially Sean's work, so I am keeping the patch set version
> > numbering scheme going.
> > 
> > Tested on Cavium ThunderX system with 4 Root Complexes containing 50
> > devices/bridges provisioned with EA.
> > 
> > Here is Sean's description of the patches:
> > 
> > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > resources for PCI devices & bridges. It can be used instead of the 
> > traditional
> > PCI method of using BARs.
> > 
> > EA entries are hardware-initialized to a fixed address.
> > Unlike BARs, regions described by EA are cannot be moved.
> > Because of this, only devices which are permanently connected to the PCI
> > bus can use EA. A removable PCI card must not use EA.
> > 
> > This patchset adds support for using EA entries instead of BARs on Root
> > Complex Integrated Endpoints.
> > 
> > The Enhanced Allocation ECN is publicly available here:
> > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Alloca
> > tion_23_Oct_2014_Final.pdf
> > 
> > 
> > Changes from V1:
> > - Use generic PCI resource claim functions (instead of EA-specific
> > functions)
> > - Only add support for RCiEPs (instead of all devices).
> > - Removed some debugging messages leftover from early testing.
> > 
> > Changes from V2 (By David Daney):
> > - Add ea_cap to struct pci_device, to aid in finding the EA capability.
> > - Factored EA entity decoding into a separate function.
> > - Add functions to find EA entities by BEI or Property.
> > - Add handling of EA provisioned bridges.
> > - Add handling of EA SRIOV BARs.
> > - Try to assign proper resource parent so that SRIOV device creation
> > can occur.
> > 
> > Changes from V3 (By David Daney):
> > - Discarded V3 changes and started over fresh based on Sean's V2.
> > - Add more support/checking for Entry Properties.
> > - Allow EA behind bridges.
> > - Rewrite some error messages.
> > - Add patch 3/5 to prevent resizing, and better handle
> >   assigning, of fixed EA resources.
> > - Add patch 4/5 to handle EA provisioned SRIOV devices.
> > - Add patch 5/5 to handle EA provisioned bridges.
> > 
> > Changes from V4 (By David Daney):
> > - Drop patch 5/5 to handle EA provisioned bridges.
> > - Drop cases for bridge resources in 2/5.
> > - Drop unnecessary fallback resource parent handling in 3/5
> > - Small code formatting improvements.
> > 
> > David Daney (2):
> >   PCI: Handle IORESOURCE_PCI_FIXED when sizing and assigning resources.
> >   PCI: Handle Enhanced Allocation (EA) capability for SRIOV devices.
> > 
> > Sean O. Stalley (2):
> >   PCI: Add Enhanced Allocation register entries
> >   PCI: Add support for Enhanced Allocation devices
> > 
> >  drivers/pci/iov.c |  11 ++-
> >  drivers/pci/pci.c | 189
> > ++
> >  drivers/pci/pci.h |   1 +
> >  drivers/pci/probe.c   |   3 +
> >  drivers/pci/setup-bus.c   |  50 ++-
> >  include/uapi/linux/pci_regs.h |  44 +-
> >  6 files changed, 292 insertions(+), 6 deletions(-)
> > 
> > --
> > 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 0/4] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

Signed-off-by: Sean O. Stalley <sean.stal...@intel.com>

I tested it out with the QEMU EA Patches here:
[https://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg00348.html]

Also, I found 1 trivial typo in the commit message of PATCH 1/4:
"Signed-off-by: Signed-off-by: David Daney <david.da...@cavium.com>"

-Sean

On Wed, Oct 07, 2015 at 06:44:52AM -0700, Stalley, Sean wrote:
> [PATCH 3/4 & 4/4] Acked-by: Sean O. Stalley <sean.stal...@intel.com>
> 
> I won't be able to test it out until next week, but I like how it looks :)
> 
> Thanks Again,
> Sean
> 
> > -Original Message-
> > From: David Daney [mailto:ddaney.c...@gmail.com]
> > Sent: Tuesday, October 06, 2015 4:51 PM
> > To: linux-kernel@vger.kernel.org; linux-...@vger.kernel.org; Bjorn Helgaas;
> > Michael S. Tsirkin; Rafał Miłecki; linux-...@vger.kernel.org; Stalley, Sean;
> > ying...@kernel.org; rajatxj...@gmail.com; gong.c...@linux.intel.com
> > Cc: David Daney
> > Subject: [PATCH v5 0/4] PCI: Add support for PCI Enhanced Allocation
> > "BARs"
> > 
> > From: David Daney <david.da...@cavium.com>
> > 
> > The original patches are from Sean O. Stalley. I made a few tweaks, but feel
> > that it is substancially Sean's work, so I am keeping the patch set version
> > numbering scheme going.
> > 
> > Tested on Cavium ThunderX system with 4 Root Complexes containing 50
> > devices/bridges provisioned with EA.
> > 
> > Here is Sean's description of the patches:
> > 
> > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > resources for PCI devices & bridges. It can be used instead of the 
> > traditional
> > PCI method of using BARs.
> > 
> > EA entries are hardware-initialized to a fixed address.
> > Unlike BARs, regions described by EA are cannot be moved.
> > Because of this, only devices which are permanently connected to the PCI
> > bus can use EA. A removable PCI card must not use EA.
> > 
> > This patchset adds support for using EA entries instead of BARs on Root
> > Complex Integrated Endpoints.
> > 
> > The Enhanced Allocation ECN is publicly available here:
> > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Alloca
> > tion_23_Oct_2014_Final.pdf
> > 
> > 
> > Changes from V1:
> > - Use generic PCI resource claim functions (instead of EA-specific
> > functions)
> > - Only add support for RCiEPs (instead of all devices).
> > - Removed some debugging messages leftover from early testing.
> > 
> > Changes from V2 (By David Daney):
> > - Add ea_cap to struct pci_device, to aid in finding the EA capability.
> > - Factored EA entity decoding into a separate function.
> > - Add functions to find EA entities by BEI or Property.
> > - Add handling of EA provisioned bridges.
> > - Add handling of EA SRIOV BARs.
> > - Try to assign proper resource parent so that SRIOV device creation
> > can occur.
> > 
> > Changes from V3 (By David Daney):
> > - Discarded V3 changes and started over fresh based on Sean's V2.
> > - Add more support/checking for Entry Properties.
> > - Allow EA behind bridges.
> > - Rewrite some error messages.
> > - Add patch 3/5 to prevent resizing, and better handle
> >   assigning, of fixed EA resources.
> > - Add patch 4/5 to handle EA provisioned SRIOV devices.
> > - Add patch 5/5 to handle EA provisioned bridges.
> > 
> > Changes from V4 (By David Daney):
> > - Drop patch 5/5 to handle EA provisioned bridges.
> > - Drop cases for bridge resources in 2/5.
> > - Drop unnecessary fallback resource parent handling in 3/5
> > - Small code formatting improvements.
> > 
> > David Daney (2):
> >   PCI: Handle IORESOURCE_PCI_FIXED when sizing and assigning resources.
> >   PCI: Handle Enhanced Allocation (EA) capability for SRIOV devices.
> > 
> > Sean O. Stalley (2):
> >   PCI: Add Enhanced Allocation register entries
> >   PCI: Add support for Enhanced Allocation devices
> > 
> >  drivers/pci/iov.c |  11 ++-
> >  drivers/pci/pci.c | 189
> > ++
> >  drivers/pci/pci.h |   1 +
> >  drivers/pci/probe.c   |   3 +
> >  drivers/pci/setup-bus.c   |  50 ++-
> >  include/uapi/linux/pci_regs.h |  44 +-
> >  6 files changed, 292 insertions(+), 6 deletions(-)
> > 
> > --
> > 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 0/5] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Mon, Oct 05, 2015 at 06:17:20PM -0700, David Daney wrote:
> On 10/05/2015 04:05 PM, Sean O. Stalley wrote:
> >On Fri, Oct 02, 2015 at 08:16:48PM -0700, Yinghai Lu wrote:
> >>On Fri, Oct 2, 2015 at 3:37 PM, David Daney  wrote:
> >>>From: David Daney 
> >>>
> >>>PCI Enhanced Allocation is a new method of allocating MMIO & IO
> >>>resources for PCI devices & bridges. It can be used instead
> >>>of the traditional PCI method of using BARs.
> >>>
> >>>EA entries are hardware-initialized to a fixed address.
> >>>Unlike BARs, regions described by EA are cannot be moved.
> >>>Because of this, only devices which are permanently connected to
> >>>the PCI bus can use EA. A removable PCI card must not use EA.
> >>>
> >>>The Enhanced Allocation ECN is publicly available here:
> >>>https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> >>
> >>Looks like the EA will support more than just fixed address later.
> >>
> >>"Enhanced Allocation is an optional Conventional PCI Capability that
> >>may be implemented by
> >>Functions to indicate fixed (non reprogrammable) I/O and memory ranges
> >>assigned to the
> >>Function, as well as supporting new resource “type” definitions and
> >>future extensibility to also
> >>support reprogrammable allocations."
> >>
> >>so I would prefer to think more to make frame configurable to leave
> >>space for that.
> >>
> >>Bjorn,
> >>
> >>I wonder if we need to revive the add-on resource support patchset
> >>that i suggested couple years ago,
> >>so we can extend it to support EA features.
> >>
> >>URL: https://lkml.org/lkml/2012/3/19/86
> >>
> >>Thanks
> >>
> >>Yinghai
> >
> >This might be useful for fixed resources as well.
> >
> >For some BEI values, EA allows for an arbitrary number of EA entries.
> 
> I think this is true only for BEI = 6 which is for type-1 config
> space only (i.e. for bridges)

It's true for a BEI of 6 or 7.

>From the ECN (specifically section 6.9.1.3):

Rules for use of BEI field:
...
It is permitted for an arbitrary number of entries to assign a BEI of 6 
or 7.

> I am thinking about splitting out the bridge part of the patch set,
> as my systems work fine without explicitly assigning bridge
> resources via EA.  That would allow us to more forward with the
> patches that are less controversial, and spend more time hashing out
> the proper approach to take with bridges.

I like this idea. Adding support for Endpoints will be much simpler than
bridges.

> 
> >For PF & VF resource ranges, it allows 2 ranges.
> 
> I don't really understand what you are saying here.  My reading of
> the spec. is that BEI[0..5] are PF BARs and each may have any of the
> properties that are allowed for normal BARs (io,
> memory-nonprefetchable, memory-prefetchable).  BEI[9..14] are VF
> BARs, and likewise may have any of the properties that are alloed
> for normal VF bars (memory-nonprefetchable, memory-prefetchable)


>From the ECN (specifically section 6.9.1.3):

Rules for use of BEI field:
...

For Memory or I/O BARs where the Primary or Secondary Property is 00h, 01h or
02h, it is permitted to assign the same BEI in the range of 0-5 once for a 
range where
Base + MaxOffset is below 4GB, and again for a range where Base + MaxOffset is
greater than 4GB; It is not otherwise permitted to assign the same BEI in the 
range 0-
5 for more than one entry.

For Virtual Function BARs where the Primary or Secondary Property is 03h or 04h 
it
is permitted to assign the same BEI in the range of 0-5 once for a range where 
Base +
MaxOffset is below 4GB, and again for a range where Base + MaxOffset is greater
than 4GB; It is not otherwise permitted to assign the same BEI in the range 0-5 
for
more than one VF entry.

(Theres a typo in the VF rule. I think the '0-5' range should be '9-14' for VFs)

> I guess in theory EA allows you to allocate 6 64-bit BARs, where you
> would be limited to only 3 64-bit normal BARs

I guess so, I never thought about that.

> >(one below the 4GB boundry, and one above).
> >I don't think the current pci_dev struct can handle that many resources.
> >
> >-Sean
> >

Let me know if you want any help with the patchset. I'll do what I can.
I'm in today, but I'm out Wed-Fri this week.

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 0/5] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Mon, Oct 05, 2015 at 06:17:20PM -0700, David Daney wrote:
> On 10/05/2015 04:05 PM, Sean O. Stalley wrote:
> >On Fri, Oct 02, 2015 at 08:16:48PM -0700, Yinghai Lu wrote:
> >>On Fri, Oct 2, 2015 at 3:37 PM, David Daney <ddaney.c...@gmail.com> wrote:
> >>>From: David Daney <david.da...@cavium.com>
> >>>
> >>>PCI Enhanced Allocation is a new method of allocating MMIO & IO
> >>>resources for PCI devices & bridges. It can be used instead
> >>>of the traditional PCI method of using BARs.
> >>>
> >>>EA entries are hardware-initialized to a fixed address.
> >>>Unlike BARs, regions described by EA are cannot be moved.
> >>>Because of this, only devices which are permanently connected to
> >>>the PCI bus can use EA. A removable PCI card must not use EA.
> >>>
> >>>The Enhanced Allocation ECN is publicly available here:
> >>>https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> >>
> >>Looks like the EA will support more than just fixed address later.
> >>
> >>"Enhanced Allocation is an optional Conventional PCI Capability that
> >>may be implemented by
> >>Functions to indicate fixed (non reprogrammable) I/O and memory ranges
> >>assigned to the
> >>Function, as well as supporting new resource “type” definitions and
> >>future extensibility to also
> >>support reprogrammable allocations."
> >>
> >>so I would prefer to think more to make frame configurable to leave
> >>space for that.
> >>
> >>Bjorn,
> >>
> >>I wonder if we need to revive the add-on resource support patchset
> >>that i suggested couple years ago,
> >>so we can extend it to support EA features.
> >>
> >>URL: https://lkml.org/lkml/2012/3/19/86
> >>
> >>Thanks
> >>
> >>Yinghai
> >
> >This might be useful for fixed resources as well.
> >
> >For some BEI values, EA allows for an arbitrary number of EA entries.
> 
> I think this is true only for BEI = 6 which is for type-1 config
> space only (i.e. for bridges)

It's true for a BEI of 6 or 7.

>From the ECN (specifically section 6.9.1.3):

Rules for use of BEI field:
...
It is permitted for an arbitrary number of entries to assign a BEI of 6 
or 7.

> I am thinking about splitting out the bridge part of the patch set,
> as my systems work fine without explicitly assigning bridge
> resources via EA.  That would allow us to more forward with the
> patches that are less controversial, and spend more time hashing out
> the proper approach to take with bridges.

I like this idea. Adding support for Endpoints will be much simpler than
bridges.

> 
> >For PF & VF resource ranges, it allows 2 ranges.
> 
> I don't really understand what you are saying here.  My reading of
> the spec. is that BEI[0..5] are PF BARs and each may have any of the
> properties that are allowed for normal BARs (io,
> memory-nonprefetchable, memory-prefetchable).  BEI[9..14] are VF
> BARs, and likewise may have any of the properties that are alloed
> for normal VF bars (memory-nonprefetchable, memory-prefetchable)


>From the ECN (specifically section 6.9.1.3):

Rules for use of BEI field:
...

For Memory or I/O BARs where the Primary or Secondary Property is 00h, 01h or
02h, it is permitted to assign the same BEI in the range of 0-5 once for a 
range where
Base + MaxOffset is below 4GB, and again for a range where Base + MaxOffset is
greater than 4GB; It is not otherwise permitted to assign the same BEI in the 
range 0-
5 for more than one entry.

For Virtual Function BARs where the Primary or Secondary Property is 03h or 04h 
it
is permitted to assign the same BEI in the range of 0-5 once for a range where 
Base +
MaxOffset is below 4GB, and again for a range where Base + MaxOffset is greater
than 4GB; It is not otherwise permitted to assign the same BEI in the range 0-5 
for
more than one VF entry.

(Theres a typo in the VF rule. I think the '0-5' range should be '9-14' for VFs)

> I guess in theory EA allows you to allocate 6 64-bit BARs, where you
> would be limited to only 3 64-bit normal BARs

I guess so, I never thought about that.

> >(one below the 4GB boundry, and one above).
> >I don't think the current pci_dev struct can handle that many resources.
> >
> >-Sean
> >

Let me know if you want any help with the patchset. I'll do what I can.
I'm in today, but I'm out Wed-Fri this week.

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 0/5] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Fri, Oct 02, 2015 at 08:16:48PM -0700, Yinghai Lu wrote:
> On Fri, Oct 2, 2015 at 3:37 PM, David Daney  wrote:
> > From: David Daney 
> >
> > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > resources for PCI devices & bridges. It can be used instead
> > of the traditional PCI method of using BARs.
> >
> > EA entries are hardware-initialized to a fixed address.
> > Unlike BARs, regions described by EA are cannot be moved.
> > Because of this, only devices which are permanently connected to
> > the PCI bus can use EA. A removable PCI card must not use EA.
> >
> > The Enhanced Allocation ECN is publicly available here:
> > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> 
> Looks like the EA will support more than just fixed address later.
> 
> "Enhanced Allocation is an optional Conventional PCI Capability that
> may be implemented by
> Functions to indicate fixed (non reprogrammable) I/O and memory ranges
> assigned to the
> Function, as well as supporting new resource “type” definitions and
> future extensibility to also
> support reprogrammable allocations."
> 
> so I would prefer to think more to make frame configurable to leave
> space for that.
> 
> Bjorn,
> 
> I wonder if we need to revive the add-on resource support patchset
> that i suggested couple years ago,
> so we can extend it to support EA features.
> 
> URL: https://lkml.org/lkml/2012/3/19/86
> 
> Thanks
> 
> Yinghai

This might be useful for fixed resources as well.

For some BEI values, EA allows for an arbitrary number of EA entries.
For PF & VF resource ranges, it allows 2 ranges.
(one below the 4GB boundry, and one above).
I don't think the current pci_dev struct can handle that many resources.

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 5/5] PCI: Handle Enhanced Allocation (EA) capability for bridges

[Sean O. Stalley]

On Fri, Oct 02, 2015 at 03:37:56PM -0700, David Daney wrote:
> From: David Daney 
> 
> PCI bridges may have their properties be specified via EA entries.
> 
> Extend the EA parser to extract the bridge resources, and modify
> pci_read_bridge_{io,mmio,mmio_pref}() to use resources previously
> obtained via EA.
> 
> Save the offset to the EA capability in struct pci_dev, and use it to
> easily find the EA bridge subordinate and secondary bus numbers.
> 
> When assigning the bridge resources a couple of changes are required
> so that the EA obtained IORESOURCE_PCI_FIXED are not resized, and
> correctly linked into the resource tree.
> 
> 1) In pbus_size_mem() do not attempt to resize the bridge resources if
>they are marked as IORESOURCE_PCI_FIXED.
> 
> 2) In pci_bus_alloc_from_region()for IORESOURCE_PCI_FIXED resources, just
>try to request the resource as is, without attempting to resize it.
> 
> Signed-off-by: David Daney 
> ---
>  drivers/pci/bus.c   |  7 +++
>  drivers/pci/pci.c   | 13 +
>  drivers/pci/probe.c | 31 +--
>  drivers/pci/setup-bus.c |  3 +++
>  include/linux/pci.h |  1 +
>  5 files changed, 53 insertions(+), 2 deletions(-)
> @@ -801,8 +813,23 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev 
> *dev, int max, int pass)
>  
>   pci_read_config_dword(dev, PCI_PRIMARY_BUS, );
>   primary = buses & 0xFF;
> - secondary = (buses >> 8) & 0xFF;
> - subordinate = (buses >> 16) & 0xFF;
> + if (dev->ea_cap) {
> + u32 dw1;
> +
> + pci_read_config_dword(dev, dev->ea_cap + 4, );
> + if (dw1 & 0xFF)
> + secondary = dw1 & 0xFF;
> + else
> + secondary = (buses >> 8) & 0xFF;
> +
> + if ((dw1 >> 8) & 0xFF)
> + subordinate = (dw1 >> 8) & 0xFF;
> + else
> + subordinate = (buses >> 16) & 0xFF;
> + } else {
> + secondary = (buses >> 8) & 0xFF;
> + subordinate = (buses >> 16) & 0xFF;
> + }

We can refactor this to make it cleaner/more compact. from V3 review:


secondary = (buses >> 8) & 0xFF;
subordinate = (buses >> 16) & 0xFF;
if (dev->ea_cap) {
u32 sdw;

pci_read_config_dword(dev, dev->ea_cap + 4, );

if (sdw & 0xFF)
secondary = sdw & 0xFF;

sdw >>= 8;
if (sdw & 0xFF)
subordinate = sdw & 0xFF;
}


-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 3/5] PCI: Handle IORESOURCE_PCI_FIXED when sizing and assigning resources.

[Sean O. Stalley]

On Fri, Oct 02, 2015 at 08:00:20PM -0700, Yinghai Lu wrote:
> On Fri, Oct 2, 2015 at 4:38 PM, David Daney  wrote:
> > On 10/02/2015 04:14 PM, Yinghai Lu wrote:
> >>
> >> https://patchwork.kernel.org/patch/7304371/
> >> [v6,06/53] PCI: Claim fixed resource during remove/rescan path
> >
> >
> > This one is interesting, but I don't think it will work.
> >
> > pci_claim_resource() calls pci_find_parent_resource(), which will fail in
> > important use cases.
> >
> > It is perfectly legal for a bridge provisioned by EA to not specify any
> > resources.  In this case we must walk up the bus tree until we find
> > something that contains the device resource, and can thus be a parent.
> >
> > That is a big part of what my patch is doing.
> 
> looks we need another resource flags for EA in addition to
> FIXED as it could out side of bridge MMIO range.

I think I get what you are saying, but let me check.

If a bridge gets reconfigured, it needs to be able to tell if this resource is 
EA or not.
If it is a fixed EA resource, it doesn't need to include it in its MMIO range.
If it is fixed for some other reason,
we should make sure the bridges MMIO range includes the fixed region.

You are suggesting that we should add a flag for EA, so if a bridge tries to 
move,
the bridge knows weather or not the resource is EA,
and therefore if it needs to worry about the resource when setting it's range?

Is that what you are trying to say?

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 3/5] PCI: Handle IORESOURCE_PCI_FIXED when sizing and assigning resources.

[Sean O. Stalley]

On Fri, Oct 02, 2015 at 03:37:54PM -0700, David Daney wrote:

...

> +/*
> + * Try to assign any resources marked as IORESOURCE_PCI_FIXED, as they
> + * are skipped by pbus_assign_resources_sorted().
> + */
> +static void pdev_assign_fixed_resources(struct pci_dev *dev)
> +{
> + int i;
> +
> + for (i = 0; i <  PCI_NUM_RESOURCES; i++) {
> + struct pci_bus *b;
> + struct resource *r = >resource[i];
> +
> + if (r->parent || !(r->flags & IORESOURCE_PCI_FIXED) ||
> + !(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
> + continue;
> +
> + b = dev->bus;
> + while (b && !r->parent) {
> + assign_fixed_resource_on_bus(b, r);
> + b = b->parent;
> + }
> + if (!r->parent) {
> + /*
> +  * If that didn't work, try to fallback to the
> +  * ultimate parent.
> +  */
> + if (r->flags &  IORESOURCE_IO)
> + request_resource(_resource, r);
> + else
> + request_resource(_resource, r);
> + }

I don't think this check is necessary.
assign_fixed_resource_on_bus() should find these resources when it reaches the 
top bus.
(since since the root bus contains these resources).

> + }
> +}

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 5/5] PCI: Handle Enhanced Allocation (EA) capability for bridges

[Sean O. Stalley]

On Fri, Oct 02, 2015 at 03:37:56PM -0700, David Daney wrote:
> From: David Daney 
> 
> PCI bridges may have their properties be specified via EA entries.
> 
> Extend the EA parser to extract the bridge resources, and modify
> pci_read_bridge_{io,mmio,mmio_pref}() to use resources previously
> obtained via EA.
> 
> Save the offset to the EA capability in struct pci_dev, and use it to
> easily find the EA bridge subordinate and secondary bus numbers.
> 
> When assigning the bridge resources a couple of changes are required
> so that the EA obtained IORESOURCE_PCI_FIXED are not resized, and
> correctly linked into the resource tree.
> 
> 1) In pbus_size_mem() do not attempt to resize the bridge resources if
>they are marked as IORESOURCE_PCI_FIXED.
> 
> 2) In pci_bus_alloc_from_region()for IORESOURCE_PCI_FIXED resources, just
>try to request the resource as is, without attempting to resize it.
> 
> Signed-off-by: David Daney 
> ---
>  drivers/pci/bus.c   |  7 +++
>  drivers/pci/pci.c   | 13 +
>  drivers/pci/probe.c | 31 +--
>  drivers/pci/setup-bus.c |  3 +++
>  include/linux/pci.h |  1 +
>  5 files changed, 53 insertions(+), 2 deletions(-)
> @@ -801,8 +813,23 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev 
> *dev, int max, int pass)
>  
>   pci_read_config_dword(dev, PCI_PRIMARY_BUS, );
>   primary = buses & 0xFF;
> - secondary = (buses >> 8) & 0xFF;
> - subordinate = (buses >> 16) & 0xFF;
> + if (dev->ea_cap) {
> + u32 dw1;
> +
> + pci_read_config_dword(dev, dev->ea_cap + 4, );
> + if (dw1 & 0xFF)
> + secondary = dw1 & 0xFF;
> + else
> + secondary = (buses >> 8) & 0xFF;
> +
> + if ((dw1 >> 8) & 0xFF)
> + subordinate = (dw1 >> 8) & 0xFF;
> + else
> + subordinate = (buses >> 16) & 0xFF;
> + } else {
> + secondary = (buses >> 8) & 0xFF;
> + subordinate = (buses >> 16) & 0xFF;
> + }

We can refactor this to make it cleaner/more compact. from V3 review:


secondary = (buses >> 8) & 0xFF;
subordinate = (buses >> 16) & 0xFF;
if (dev->ea_cap) {
u32 sdw;

pci_read_config_dword(dev, dev->ea_cap + 4, );

if (sdw & 0xFF)
secondary = sdw & 0xFF;

sdw >>= 8;
if (sdw & 0xFF)
subordinate = sdw & 0xFF;
}


-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 3/5] PCI: Handle IORESOURCE_PCI_FIXED when sizing and assigning resources.

[Sean O. Stalley]

On Fri, Oct 02, 2015 at 03:37:54PM -0700, David Daney wrote:

...

> +/*
> + * Try to assign any resources marked as IORESOURCE_PCI_FIXED, as they
> + * are skipped by pbus_assign_resources_sorted().
> + */
> +static void pdev_assign_fixed_resources(struct pci_dev *dev)
> +{
> + int i;
> +
> + for (i = 0; i <  PCI_NUM_RESOURCES; i++) {
> + struct pci_bus *b;
> + struct resource *r = >resource[i];
> +
> + if (r->parent || !(r->flags & IORESOURCE_PCI_FIXED) ||
> + !(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
> + continue;
> +
> + b = dev->bus;
> + while (b && !r->parent) {
> + assign_fixed_resource_on_bus(b, r);
> + b = b->parent;
> + }
> + if (!r->parent) {
> + /*
> +  * If that didn't work, try to fallback to the
> +  * ultimate parent.
> +  */
> + if (r->flags &  IORESOURCE_IO)
> + request_resource(_resource, r);
> + else
> + request_resource(_resource, r);
> + }

I don't think this check is necessary.
assign_fixed_resource_on_bus() should find these resources when it reaches the 
top bus.
(since since the root bus contains these resources).

> + }
> +}

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 3/5] PCI: Handle IORESOURCE_PCI_FIXED when sizing and assigning resources.

[Sean O. Stalley]

On Fri, Oct 02, 2015 at 08:00:20PM -0700, Yinghai Lu wrote:
> On Fri, Oct 2, 2015 at 4:38 PM, David Daney  wrote:
> > On 10/02/2015 04:14 PM, Yinghai Lu wrote:
> >>
> >> https://patchwork.kernel.org/patch/7304371/
> >> [v6,06/53] PCI: Claim fixed resource during remove/rescan path
> >
> >
> > This one is interesting, but I don't think it will work.
> >
> > pci_claim_resource() calls pci_find_parent_resource(), which will fail in
> > important use cases.
> >
> > It is perfectly legal for a bridge provisioned by EA to not specify any
> > resources.  In this case we must walk up the bus tree until we find
> > something that contains the device resource, and can thus be a parent.
> >
> > That is a big part of what my patch is doing.
> 
> looks we need another resource flags for EA in addition to
> FIXED as it could out side of bridge MMIO range.

I think I get what you are saying, but let me check.

If a bridge gets reconfigured, it needs to be able to tell if this resource is 
EA or not.
If it is a fixed EA resource, it doesn't need to include it in its MMIO range.
If it is fixed for some other reason,
we should make sure the bridges MMIO range includes the fixed region.

You are suggesting that we should add a flag for EA, so if a bridge tries to 
move,
the bridge knows weather or not the resource is EA,
and therefore if it needs to worry about the resource when setting it's range?

Is that what you are trying to say?

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 0/5] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Fri, Oct 02, 2015 at 08:16:48PM -0700, Yinghai Lu wrote:
> On Fri, Oct 2, 2015 at 3:37 PM, David Daney  wrote:
> > From: David Daney 
> >
> > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > resources for PCI devices & bridges. It can be used instead
> > of the traditional PCI method of using BARs.
> >
> > EA entries are hardware-initialized to a fixed address.
> > Unlike BARs, regions described by EA are cannot be moved.
> > Because of this, only devices which are permanently connected to
> > the PCI bus can use EA. A removable PCI card must not use EA.
> >
> > The Enhanced Allocation ECN is publicly available here:
> > https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> 
> Looks like the EA will support more than just fixed address later.
> 
> "Enhanced Allocation is an optional Conventional PCI Capability that
> may be implemented by
> Functions to indicate fixed (non reprogrammable) I/O and memory ranges
> assigned to the
> Function, as well as supporting new resource “type” definitions and
> future extensibility to also
> support reprogrammable allocations."
> 
> so I would prefer to think more to make frame configurable to leave
> space for that.
> 
> Bjorn,
> 
> I wonder if we need to revive the add-on resource support patchset
> that i suggested couple years ago,
> so we can extend it to support EA features.
> 
> URL: https://lkml.org/lkml/2012/3/19/86
> 
> Thanks
> 
> Yinghai

This might be useful for fixed resources as well.

For some BEI values, EA allows for an arbitrary number of EA entries.
For PF & VF resource ranges, it allows 2 ranges.
(one below the 4GB boundry, and one above).
I don't think the current pci_dev struct can handle that many resources.

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 0/5] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

Hi David,

I did a quick look through & overall I what you have done.
I will try to find some time to do a full review early next week.

Thanks Again,
Sean

On Fri, Oct 02, 2015 at 03:37:51PM -0700, David Daney wrote:
> From: David Daney 
> 
> The original patches are from Sean O. Stalley. I made a few tweaks,
> but feel that it is substancially Sean's work, so I am keeping the
> patch set version numbering scheme going.
> 
> Tested on Cavium ThunderX system with 4 Root Complexes containing 50
> devices/bridges provisioned with EA.
> 
> Here is Sean's description of the patches:
> 
> PCI Enhanced Allocation is a new method of allocating MMIO & IO
> resources for PCI devices & bridges. It can be used instead
> of the traditional PCI method of using BARs.
> 
> EA entries are hardware-initialized to a fixed address.
> Unlike BARs, regions described by EA are cannot be moved.
> Because of this, only devices which are permanently connected to
> the PCI bus can use EA. A removable PCI card must not use EA.
> 
> This patchset adds support for using EA entries instead of BARs
> on Root Complex Integrated Endpoints.
> 
> The Enhanced Allocation ECN is publicly available here:
> https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> 
> 
> Changes from V1:
>   - Use generic PCI resource claim functions (instead of EA-specific 
> functions)
>   - Only add support for RCiEPs (instead of all devices).
>   - Removed some debugging messages leftover from early testing.
> 
> Changes from V2 (By David Daney):
>   - Add ea_cap to struct pci_device, to aid in finding the EA capability.
>   - Factored EA entity decoding into a separate function.
>   - Add functions to find EA entities by BEI or Property.
>   - Add handling of EA provisioned bridges.
>   - Add handling of EA SRIOV BARs.
>   - Try to assign proper resource parent so that SRIOV device creation 
> can occur.
> 
> Changes from V3 (By David Daney):
>   - Discarded V3 changes and started over fresh based on Sean's V2.
>   - Add more support/checking for Entry Properties.
>   - Allow EA behind bridges.
>   - Rewrite some error messages.
>   - Add patch 3/5 to prevent resizing, and better handle
>   assigning, of fixed EA resources.
>   - Add patch 4/5 to handle EA provisioned SRIOV devices.
>   - Add patch 5/5 to handle EA provisioned bridges.
> 
> David Daney (3):
>   PCI: Handle IORESOURCE_PCI_FIXED when sizing and assigning resources.
>   PCI: Handle Enhanced Allocation (EA) capability for SRIOV devices.
>   PCI: Handle Enhanced Allocation (EA) capability for bridges
> 
> Sean O. Stalley (2):
>   PCI: Add Enhanced Allocation register entries
>   PCI: Add support for Enhanced Allocation devices
> 
>  drivers/pci/bus.c |   7 ++
>  drivers/pci/iov.c |  11 ++-
>  drivers/pci/pci.c | 202 
> ++
>  drivers/pci/pci.h |   1 +
>  drivers/pci/probe.c   |  34 ++-
>  drivers/pci/setup-bus.c   |  63 -
>  include/linux/pci.h   |   1 +
>  include/uapi/linux/pci_regs.h |  44 -
>  8 files changed, 355 insertions(+), 8 deletions(-)
> 
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 0/5] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

Hi David,

I did a quick look through & overall I what you have done.
I will try to find some time to do a full review early next week.

Thanks Again,
Sean

On Fri, Oct 02, 2015 at 03:37:51PM -0700, David Daney wrote:
> From: David Daney <david.da...@cavium.com>
> 
> The original patches are from Sean O. Stalley. I made a few tweaks,
> but feel that it is substancially Sean's work, so I am keeping the
> patch set version numbering scheme going.
> 
> Tested on Cavium ThunderX system with 4 Root Complexes containing 50
> devices/bridges provisioned with EA.
> 
> Here is Sean's description of the patches:
> 
> PCI Enhanced Allocation is a new method of allocating MMIO & IO
> resources for PCI devices & bridges. It can be used instead
> of the traditional PCI method of using BARs.
> 
> EA entries are hardware-initialized to a fixed address.
> Unlike BARs, regions described by EA are cannot be moved.
> Because of this, only devices which are permanently connected to
> the PCI bus can use EA. A removable PCI card must not use EA.
> 
> This patchset adds support for using EA entries instead of BARs
> on Root Complex Integrated Endpoints.
> 
> The Enhanced Allocation ECN is publicly available here:
> https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> 
> 
> Changes from V1:
>   - Use generic PCI resource claim functions (instead of EA-specific 
> functions)
>   - Only add support for RCiEPs (instead of all devices).
>   - Removed some debugging messages leftover from early testing.
> 
> Changes from V2 (By David Daney):
>   - Add ea_cap to struct pci_device, to aid in finding the EA capability.
>   - Factored EA entity decoding into a separate function.
>   - Add functions to find EA entities by BEI or Property.
>   - Add handling of EA provisioned bridges.
>   - Add handling of EA SRIOV BARs.
>   - Try to assign proper resource parent so that SRIOV device creation 
> can occur.
> 
> Changes from V3 (By David Daney):
>   - Discarded V3 changes and started over fresh based on Sean's V2.
>   - Add more support/checking for Entry Properties.
>   - Allow EA behind bridges.
>   - Rewrite some error messages.
>   - Add patch 3/5 to prevent resizing, and better handle
>   assigning, of fixed EA resources.
>   - Add patch 4/5 to handle EA provisioned SRIOV devices.
>   - Add patch 5/5 to handle EA provisioned bridges.
> 
> David Daney (3):
>   PCI: Handle IORESOURCE_PCI_FIXED when sizing and assigning resources.
>   PCI: Handle Enhanced Allocation (EA) capability for SRIOV devices.
>   PCI: Handle Enhanced Allocation (EA) capability for bridges
> 
> Sean O. Stalley (2):
>   PCI: Add Enhanced Allocation register entries
>   PCI: Add support for Enhanced Allocation devices
> 
>  drivers/pci/bus.c |   7 ++
>  drivers/pci/iov.c |  11 ++-
>  drivers/pci/pci.c | 202 
> ++
>  drivers/pci/pci.h |   1 +
>  drivers/pci/probe.c   |  34 ++-
>  drivers/pci/setup-bus.c   |  63 -
>  include/linux/pci.h   |   1 +
>  include/uapi/linux/pci_regs.h |  44 -
>  8 files changed, 355 insertions(+), 8 deletions(-)
> 
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

On Tue, Sep 29, 2015 at 04:53:20PM -0700, David Daney wrote:
> On 09/29/2015 03:47 PM, Sean O. Stalley wrote:
> [...]
> >>>>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >>>>index 6a9a111..7c60b16 100644
> >>>>--- a/drivers/pci/pci.c
> >>>>+++ b/drivers/pci/pci.c
> >>>>@@ -2148,6 +2148,284 @@ void pci_pm_init(struct pci_dev *dev)
> >>>>  }
> >>>>  }
> >>>>
> >>>>+static unsigned long pci_ea_set_flags(struct pci_dev *dev, u8 prop)
> >>>>+{
> >>>>+ unsigned long flags = IORESOURCE_PCI_FIXED | IORESOURCE_SIZEALIGN;
> >>>
> >>>Why did you add the IORESOURCE_SIZEALIGN flag? EA allows for unaligned 
> >>>resources.
> >>
> >>pci_bus_assign_resources() fails causing the devices to be unusable
> >>if resource_alignment() returns zero.  The easiest fix for this was
> >>to specify IORESOURCE_SIZEALIGN.
> >>
> >>An alternative would be to change the code in setup-bus.c so that
> >>for IORESOURCE_PCI_FIXED resources, it didn't barf.
> >
> >I would do this alternative, but with a IORESOURCE_PCI_EA flag.
> >
> 
> I don't think we need IORESOURCE_PCI_EA.  If a resource is tagged as
> IORESOURCE_PCI_FIXED that means that it cannot be changed, we
> shouldn't care why it is fixed (due to an EA capability for
> example).  We just need to gracefully handle IORESOURCE_PCI_FIXED in
> the places where things currently go wrong.
> 
> David Daney
> 

I agree that we need to make sure fixed things stay fixed,
regardless of if they are fixed by EA or something else.

The question is: do we want to allow all fixed resources to be non-aligned, or 
just EA?

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

On Tue, Sep 29, 2015 at 04:53:20PM -0700, David Daney wrote:
> On 09/29/2015 03:47 PM, Sean O. Stalley wrote:
> [...]
> >>>>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >>>>index 6a9a111..7c60b16 100644
> >>>>--- a/drivers/pci/pci.c
> >>>>+++ b/drivers/pci/pci.c
> >>>>@@ -2148,6 +2148,284 @@ void pci_pm_init(struct pci_dev *dev)
> >>>>  }
> >>>>  }
> >>>>
> >>>>+static unsigned long pci_ea_set_flags(struct pci_dev *dev, u8 prop)
> >>>>+{
> >>>>+ unsigned long flags = IORESOURCE_PCI_FIXED | IORESOURCE_SIZEALIGN;
> >>>
> >>>Why did you add the IORESOURCE_SIZEALIGN flag? EA allows for unaligned 
> >>>resources.
> >>
> >>pci_bus_assign_resources() fails causing the devices to be unusable
> >>if resource_alignment() returns zero.  The easiest fix for this was
> >>to specify IORESOURCE_SIZEALIGN.
> >>
> >>An alternative would be to change the code in setup-bus.c so that
> >>for IORESOURCE_PCI_FIXED resources, it didn't barf.
> >
> >I would do this alternative, but with a IORESOURCE_PCI_EA flag.
> >
> 
> I don't think we need IORESOURCE_PCI_EA.  If a resource is tagged as
> IORESOURCE_PCI_FIXED that means that it cannot be changed, we
> shouldn't care why it is fixed (due to an EA capability for
> example).  We just need to gracefully handle IORESOURCE_PCI_FIXED in
> the places where things currently go wrong.
> 
> David Daney
> 

I agree that we need to make sure fixed things stay fixed,
regardless of if they are fixed by EA or something else.

The question is: do we want to allow all fixed resources to be non-aligned, or 
just EA?

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

More responses inline.

Thanks again,
Sean

On Tue, Sep 29, 2015 at 01:25:59PM -0700, David Daney wrote:
> Thanks for reviewing the new code.  Expect a revised patch soon.

Great! Let me know if I can help :)

> 
> Other responses below...
> 
> 
> On 09/29/2015 12:27 PM, Sean O. Stalley wrote:
> >Hi David,
> >
> >Thanks for working on this. I gave it a look and added my comments inline 
> >below.
> >I have a few concerns, but overall I like it :)
> >
> >To clarify, you are only trying to add support for Bridges that have
> >EA entries to describe the resource range of downstream devices?
> 
> Yes.  If a bridge doesn't have an EA capability, it will be treated
> as a normal bridge.
> 
> >I ask because the spec allows for bridges that don't include an EA entry for 
> >this.
> 
> Correct.  I think that should still work, but I will test it to make sure.
> 
> >
> >I think we will need to add more code to make sure we don't break 
> >hot-plugging for non-EA devices.
> 
> I cannot envision how it would go wrong, but if someone can describe
> a valid failure mode, I agree it should be handled.
> 

I'm mostly concerned with what would happen if a bridge is rescanned
and the code decides to change the bus numbers.

I don't think there is anything in place to make sure the code doesn't try to 
assign
a bus number to a non-EA bridge that an EA bridge is already using.

> >What if something happens on a non-EA bus that causes the PCI core to 
> >reassign bus numbers?
> >I don't think the current code can make sure the bus numbers stay the same.
> 
> I don't know if it makes sense to design hardware containing fixed
> BARS described by EA capabilities, but where the bus topology would
> change with respect to those devices.  So perhaps that doesn't have
> to be handled.
 
I agree, the EA portion of the bus should remain constant.
I'm just not sure the current PCI bus number code can handle constant bus 
numbers.
I'll take a look... Maybe someone with more knowledge can chime in on this.

> >
> >Also, I think we should split up the patch set to make it easier to review.
> >Maybe something like:
> > 1 - Register/Constant definitions
> > 2 - EA entry parser/RCiEP support
> > 3 - Bridge support
> > 4 - SR-IOV support
> 
> Yes, that would make it easier to understand what each part is doing.
> 
> >
> >
> >Thanks,
> >Sean
> >
> >On Mon, Sep 28, 2015 at 05:10:39PM -0700, David Daney wrote:
> >>From: "Sean O. Stalley" 
> >>
> >>Add support for devices using Enhanced Allocation entries instead of BARs.
> >>This patch allows the kernel to parse the EA Extended Capability structure
> >>in PCI configspace and claim the BAR-equivalent resources.
> >>
> >>Signed-off-by: Sean O. Stalley 
> >>[david.da...@cavium.com: Added support for bridges and SRIOV]
> >>Signed-off-by: David Daney 
> >>---
> >>  drivers/pci/iov.c   |  15 ++-
> >>  drivers/pci/pci.c   | 278 
> >> 
> >>  drivers/pci/pci.h   |   4 +
> >>  drivers/pci/probe.c |  42 +++-
> >>  drivers/pci/setup-bus.c |   3 +
> >>  include/linux/pci.h |   1 +
> >>  6 files changed, 339 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >>index ee0ebff..c034575 100644
> >>--- a/drivers/pci/iov.c
> >>+++ b/drivers/pci/iov.c
> >>@@ -435,9 +435,20 @@ found:
> >>
> >>nres = 0;
> >>for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> >>+   int ea_bar = pci_ea_find_ent_with_bei(dev,
> >>+ i + PCI_EA_BEI_VF_BAR0);
> >>+
> >>res = >resource[i + PCI_IOV_RESOURCES];
> >>-   bar64 = __pci_read_base(dev, pci_bar_unknown, res,
> >>-   pos + PCI_SRIOV_BAR + i * 4);
> >>+
> >>+   if (ea_bar > 0) {
> >>+   if (pci_ea_decode_ent(dev, ea_bar, res))
> >>+   continue;
> >
> >Do we want to decode here?
> >
> >This patch calls pci_ea_decode_ent() a lot more than I think it should...
> >I think we only want to call the decode function once per entry,
> >since it requests resources & reads configspace.
> >
> >Do you think we could find a way to only call decode() once during 
> >initialization,
> >then use info in the resource struct after that?
> >
> 
> The oth

Re: [PATCH v3 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

Hi David,

Thanks for working on this. I gave it a look and added my comments inline below.
I have a few concerns, but overall I like it :)

To clarify, you are only trying to add support for Bridges that have
EA entries to describe the resource range of downstream devices?
I ask because the spec allows for bridges that don't include an EA entry for 
this.

I think we will need to add more code to make sure we don't break hot-plugging 
for non-EA devices.
What if something happens on a non-EA bus that causes the PCI core to reassign 
bus numbers?
I don't think the current code can make sure the bus numbers stay the same.

Also, I think we should split up the patch set to make it easier to review.
Maybe something like:
1 - Register/Constant definitions
2 - EA entry parser/RCiEP support
3 - Bridge support
4 - SR-IOV support


Thanks,
Sean

On Mon, Sep 28, 2015 at 05:10:39PM -0700, David Daney wrote:
> From: "Sean O. Stalley" 
> 
> Add support for devices using Enhanced Allocation entries instead of BARs.
> This patch allows the kernel to parse the EA Extended Capability structure
> in PCI configspace and claim the BAR-equivalent resources.
> 
> Signed-off-by: Sean O. Stalley 
> [david.da...@cavium.com: Added support for bridges and SRIOV]
> Signed-off-by: David Daney 
> ---
>  drivers/pci/iov.c   |  15 ++-
>  drivers/pci/pci.c   | 278 
> 
>  drivers/pci/pci.h   |   4 +
>  drivers/pci/probe.c |  42 +++-
>  drivers/pci/setup-bus.c |   3 +
>  include/linux/pci.h |   1 +
>  6 files changed, 339 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index ee0ebff..c034575 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -435,9 +435,20 @@ found:
>  
>   nres = 0;
>   for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> + int ea_bar = pci_ea_find_ent_with_bei(dev,
> +   i + PCI_EA_BEI_VF_BAR0);
> +
>   res = >resource[i + PCI_IOV_RESOURCES];
> - bar64 = __pci_read_base(dev, pci_bar_unknown, res,
> - pos + PCI_SRIOV_BAR + i * 4);
> +
> + if (ea_bar > 0) {
> + if (pci_ea_decode_ent(dev, ea_bar, res))
> + continue;

Do we want to decode here?

This patch calls pci_ea_decode_ent() a lot more than I think it should...
I think we only want to call the decode function once per entry,
since it requests resources & reads configspace.

Do you think we could find a way to only call decode() once during 
initialization,
then use info in the resource struct after that?


> + bar64 = (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
> + } else {
> + bar64 = __pci_read_base(dev, pci_bar_unknown, res,
> + pos + PCI_SRIOV_BAR + i * 4);
> + }
> +
>   if (!res->flags)
>   continue;
>   if (resource_size(res) & (PAGE_SIZE - 1)) {
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6a9a111..7c60b16 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2148,6 +2148,284 @@ void pci_pm_init(struct pci_dev *dev)
>   }
>  }
>  
> +static unsigned long pci_ea_set_flags(struct pci_dev *dev, u8 prop)
> +{
> + unsigned long flags = IORESOURCE_PCI_FIXED | IORESOURCE_SIZEALIGN;

Why did you add the IORESOURCE_SIZEALIGN flag? EA allows for unaligned 
resources.

> +
> + switch (prop) {
> + case PCI_EA_P_MEM:
> + case PCI_EA_P_VIRT_MEM:
> + case PCI_EA_P_BRIDGE_MEM:
> + flags |= IORESOURCE_MEM;
> + break;
> + case PCI_EA_P_MEM_PREFETCH:
> + case PCI_EA_P_VIRT_MEM_PREFETCH:
> + case PCI_EA_P_BRIDGE_MEM_PREFETCH:
> + flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
> + break;
> + case PCI_EA_P_IO:
> + case PCI_EA_P_BRIDGE_IO:
> + flags |= IORESOURCE_IO;
> + break;
> + default:
> + return 0;
> + }
> +
> + return flags;
> +}
> +
> +static struct resource *pci_ea_get_resource(struct pci_dev *dev, u8 bei)
> +{
> + if (bei <= PCI_STD_RESOURCE_END)
> + return >resource[bei];
> + else if (bei == PCI_EA_BEI_ROM)
> + return >resource[PCI_ROM_RESOURCE];

For SR-IOV, you would need to handle the case where the bei is between
PCI_EA_BEI_IOV and PCI_EA_BEI_IOV_END  (9-14)

For Bridges, we need to handle the PCI_EA_BEI_BRIDGE (6) case.

> + else
> + return NULL;
> +}
> +
> +int pc

Re: [PATCH v3 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

More responses inline.

Thanks again,
Sean

On Tue, Sep 29, 2015 at 01:25:59PM -0700, David Daney wrote:
> Thanks for reviewing the new code.  Expect a revised patch soon.

Great! Let me know if I can help :)

> 
> Other responses below...
> 
> 
> On 09/29/2015 12:27 PM, Sean O. Stalley wrote:
> >Hi David,
> >
> >Thanks for working on this. I gave it a look and added my comments inline 
> >below.
> >I have a few concerns, but overall I like it :)
> >
> >To clarify, you are only trying to add support for Bridges that have
> >EA entries to describe the resource range of downstream devices?
> 
> Yes.  If a bridge doesn't have an EA capability, it will be treated
> as a normal bridge.
> 
> >I ask because the spec allows for bridges that don't include an EA entry for 
> >this.
> 
> Correct.  I think that should still work, but I will test it to make sure.
> 
> >
> >I think we will need to add more code to make sure we don't break 
> >hot-plugging for non-EA devices.
> 
> I cannot envision how it would go wrong, but if someone can describe
> a valid failure mode, I agree it should be handled.
> 

I'm mostly concerned with what would happen if a bridge is rescanned
and the code decides to change the bus numbers.

I don't think there is anything in place to make sure the code doesn't try to 
assign
a bus number to a non-EA bridge that an EA bridge is already using.

> >What if something happens on a non-EA bus that causes the PCI core to 
> >reassign bus numbers?
> >I don't think the current code can make sure the bus numbers stay the same.
> 
> I don't know if it makes sense to design hardware containing fixed
> BARS described by EA capabilities, but where the bus topology would
> change with respect to those devices.  So perhaps that doesn't have
> to be handled.
 
I agree, the EA portion of the bus should remain constant.
I'm just not sure the current PCI bus number code can handle constant bus 
numbers.
I'll take a look... Maybe someone with more knowledge can chime in on this.

> >
> >Also, I think we should split up the patch set to make it easier to review.
> >Maybe something like:
> > 1 - Register/Constant definitions
> > 2 - EA entry parser/RCiEP support
> > 3 - Bridge support
> > 4 - SR-IOV support
> 
> Yes, that would make it easier to understand what each part is doing.
> 
> >
> >
> >Thanks,
> >Sean
> >
> >On Mon, Sep 28, 2015 at 05:10:39PM -0700, David Daney wrote:
> >>From: "Sean O. Stalley" <sean.stal...@intel.com>
> >>
> >>Add support for devices using Enhanced Allocation entries instead of BARs.
> >>This patch allows the kernel to parse the EA Extended Capability structure
> >>in PCI configspace and claim the BAR-equivalent resources.
> >>
> >>Signed-off-by: Sean O. Stalley <sean.stal...@intel.com>
> >>[david.da...@cavium.com: Added support for bridges and SRIOV]
> >>Signed-off-by: David Daney <david.da...@cavium.com>
> >>---
> >>  drivers/pci/iov.c   |  15 ++-
> >>  drivers/pci/pci.c   | 278 
> >> 
> >>  drivers/pci/pci.h   |   4 +
> >>  drivers/pci/probe.c |  42 +++-
> >>  drivers/pci/setup-bus.c |   3 +
> >>  include/linux/pci.h |   1 +
> >>  6 files changed, 339 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> >>index ee0ebff..c034575 100644
> >>--- a/drivers/pci/iov.c
> >>+++ b/drivers/pci/iov.c
> >>@@ -435,9 +435,20 @@ found:
> >>
> >>nres = 0;
> >>for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> >>+   int ea_bar = pci_ea_find_ent_with_bei(dev,
> >>+ i + PCI_EA_BEI_VF_BAR0);
> >>+
> >>res = >resource[i + PCI_IOV_RESOURCES];
> >>-   bar64 = __pci_read_base(dev, pci_bar_unknown, res,
> >>-   pos + PCI_SRIOV_BAR + i * 4);
> >>+
> >>+   if (ea_bar > 0) {
> >>+   if (pci_ea_decode_ent(dev, ea_bar, res))
> >>+   continue;
> >
> >Do we want to decode here?
> >
> >This patch calls pci_ea_decode_ent() a lot more than I think it should...
> >I think we only want to call the decode function once per entry,
> >since it requests resources & reads configspace.
> >
> >Do you think we could find a way to only call decode() once during 
> >initialization,
&

Re: [PATCH v3 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

Hi David,

Thanks for working on this. I gave it a look and added my comments inline below.
I have a few concerns, but overall I like it :)

To clarify, you are only trying to add support for Bridges that have
EA entries to describe the resource range of downstream devices?
I ask because the spec allows for bridges that don't include an EA entry for 
this.

I think we will need to add more code to make sure we don't break hot-plugging 
for non-EA devices.
What if something happens on a non-EA bus that causes the PCI core to reassign 
bus numbers?
I don't think the current code can make sure the bus numbers stay the same.

Also, I think we should split up the patch set to make it easier to review.
Maybe something like:
1 - Register/Constant definitions
2 - EA entry parser/RCiEP support
3 - Bridge support
4 - SR-IOV support


Thanks,
Sean

On Mon, Sep 28, 2015 at 05:10:39PM -0700, David Daney wrote:
> From: "Sean O. Stalley" <sean.stal...@intel.com>
> 
> Add support for devices using Enhanced Allocation entries instead of BARs.
> This patch allows the kernel to parse the EA Extended Capability structure
> in PCI configspace and claim the BAR-equivalent resources.
> 
> Signed-off-by: Sean O. Stalley <sean.stal...@intel.com>
> [david.da...@cavium.com: Added support for bridges and SRIOV]
> Signed-off-by: David Daney <david.da...@cavium.com>
> ---
>  drivers/pci/iov.c   |  15 ++-
>  drivers/pci/pci.c   | 278 
> 
>  drivers/pci/pci.h   |   4 +
>  drivers/pci/probe.c |  42 +++-
>  drivers/pci/setup-bus.c |   3 +
>  include/linux/pci.h |   1 +
>  6 files changed, 339 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index ee0ebff..c034575 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -435,9 +435,20 @@ found:
>  
>   nres = 0;
>   for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> + int ea_bar = pci_ea_find_ent_with_bei(dev,
> +   i + PCI_EA_BEI_VF_BAR0);
> +
>   res = >resource[i + PCI_IOV_RESOURCES];
> - bar64 = __pci_read_base(dev, pci_bar_unknown, res,
> - pos + PCI_SRIOV_BAR + i * 4);
> +
> + if (ea_bar > 0) {
> + if (pci_ea_decode_ent(dev, ea_bar, res))
> + continue;

Do we want to decode here?

This patch calls pci_ea_decode_ent() a lot more than I think it should...
I think we only want to call the decode function once per entry,
since it requests resources & reads configspace.

Do you think we could find a way to only call decode() once during 
initialization,
then use info in the resource struct after that?


> + bar64 = (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
> + } else {
> + bar64 = __pci_read_base(dev, pci_bar_unknown, res,
> + pos + PCI_SRIOV_BAR + i * 4);
> + }
> +
>   if (!res->flags)
>   continue;
>   if (resource_size(res) & (PAGE_SIZE - 1)) {
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6a9a111..7c60b16 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2148,6 +2148,284 @@ void pci_pm_init(struct pci_dev *dev)
>   }
>  }
>  
> +static unsigned long pci_ea_set_flags(struct pci_dev *dev, u8 prop)
> +{
> + unsigned long flags = IORESOURCE_PCI_FIXED | IORESOURCE_SIZEALIGN;

Why did you add the IORESOURCE_SIZEALIGN flag? EA allows for unaligned 
resources.

> +
> + switch (prop) {
> + case PCI_EA_P_MEM:
> + case PCI_EA_P_VIRT_MEM:
> + case PCI_EA_P_BRIDGE_MEM:
> + flags |= IORESOURCE_MEM;
> + break;
> + case PCI_EA_P_MEM_PREFETCH:
> + case PCI_EA_P_VIRT_MEM_PREFETCH:
> + case PCI_EA_P_BRIDGE_MEM_PREFETCH:
> + flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
> + break;
> + case PCI_EA_P_IO:
> + case PCI_EA_P_BRIDGE_IO:
> + flags |= IORESOURCE_IO;
> + break;
> + default:
> + return 0;
> + }
> +
> + return flags;
> +}
> +
> +static struct resource *pci_ea_get_resource(struct pci_dev *dev, u8 bei)
> +{
> + if (bei <= PCI_STD_RESOURCE_END)
> + return >resource[bei];
> + else if (bei == PCI_EA_BEI_ROM)
> + return >resource[PCI_ROM_RESOURCE];

For SR-IOV, you would need to handle the case where the bei is between
PCI_EA_BEI_IOV and PCI_EA_BEI_IOV_END  (9-14)

For Bridges, we need to handle th

Re: [PATCH v2 0/2] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

Hi David,

My response is inline. Let me know if you have any questions.

Thanks for taking a look,
Sean

On Wed, Sep 23, 2015 at 05:34:13PM -0700, David Daney wrote:
> Hi Sean,
> 
> Thanks for doing this, I think we will use it for Cavium ThunderX.
> A couple of questions...
> 
> 
> On 09/23/2015 03:27 PM, Sean O. Stalley wrote:
> >PCI Enhanced Allocation is a new method of allocating MMIO & IO
> >resources for PCI devices & bridges. It can be used instead
> >of the traditional PCI method of using BARs.
> >
> >EA entries are hardware-initialized to a fixed address.
> >Unlike BARs, regions described by EA are cannot be moved.
> >Because of this, only devices which are permanently connected to
> >the PCI bus can use EA. A removable PCI card must not use EA.
> >
> >This patchset adds support for using EA entries instead of BARs
> >on Root Complex Integrated Endpoints.
> >
> >The Enhanced Allocation ECN is publicly available here:
> >https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> >
> >
> >Changes from V1:
> > - Use generic PCI resource claim functions (instead of EA-specific 
> > functions)
> > - Only add support for RCiEPs (instead of all devices).
> 
> Why not all devices?  The spec. allows for EA on devices behind bridges.

The short answer is that adding support for EA bridges would be a much larger 
change.
EA bridges can do some weird stuff, like fixing bus numbers & using resources 
not in the base+limit window.

See the conversation I had with Bjorn on the original patchset for the details.
[ https://lkml.org/lkml/2015/9/3/497 ]

> > - Removed some debugging messages leftover from early testing.
> >
> >
> >Sean O. Stalley (2):
> >   PCI: Add Enhanced Allocation register entries
> >   PCI: Add support for Enhanced Allocation devices
> >
> >  drivers/pci/pci.c | 174 
> > ++
> >  drivers/pci/pci.h |   1 +
> >  drivers/pci/probe.c   |   3 +
> >  include/uapi/linux/pci_regs.h |  40 +-
> >  4 files changed, 217 insertions(+), 1 deletion(-)
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/2] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

Hi David,

My response is inline. Let me know if you have any questions.

Thanks for taking a look,
Sean

On Wed, Sep 23, 2015 at 05:34:13PM -0700, David Daney wrote:
> Hi Sean,
> 
> Thanks for doing this, I think we will use it for Cavium ThunderX.
> A couple of questions...
> 
> 
> On 09/23/2015 03:27 PM, Sean O. Stalley wrote:
> >PCI Enhanced Allocation is a new method of allocating MMIO & IO
> >resources for PCI devices & bridges. It can be used instead
> >of the traditional PCI method of using BARs.
> >
> >EA entries are hardware-initialized to a fixed address.
> >Unlike BARs, regions described by EA are cannot be moved.
> >Because of this, only devices which are permanently connected to
> >the PCI bus can use EA. A removable PCI card must not use EA.
> >
> >This patchset adds support for using EA entries instead of BARs
> >on Root Complex Integrated Endpoints.
> >
> >The Enhanced Allocation ECN is publicly available here:
> >https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf
> >
> >
> >Changes from V1:
> > - Use generic PCI resource claim functions (instead of EA-specific 
> > functions)
> > - Only add support for RCiEPs (instead of all devices).
> 
> Why not all devices?  The spec. allows for EA on devices behind bridges.

The short answer is that adding support for EA bridges would be a much larger 
change.
EA bridges can do some weird stuff, like fixing bus numbers & using resources 
not in the base+limit window.

See the conversation I had with Bjorn on the original patchset for the details.
[ https://lkml.org/lkml/2015/9/3/497 ]

> > - Removed some debugging messages leftover from early testing.
> >
> >
> >Sean O. Stalley (2):
> >   PCI: Add Enhanced Allocation register entries
> >   PCI: Add support for Enhanced Allocation devices
> >
> >  drivers/pci/pci.c | 174 
> > ++
> >  drivers/pci/pci.h |   1 +
> >  drivers/pci/probe.c   |   3 +
> >  include/uapi/linux/pci_regs.h |  40 +-
> >  4 files changed, 217 insertions(+), 1 deletion(-)
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/2] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Wed, Sep 23, 2015 at 04:26:56PM -0700, Yinghai Lu wrote:
> On Wed, Sep 23, 2015 at 3:27 PM, Sean O. Stalley  
> wrote:
> > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > resources for PCI devices & bridges. It can be used instead
> > of the traditional PCI method of using BARs.
> >
> > EA entries are hardware-initialized to a fixed address.
> > Unlike BARs, regions described by EA are cannot be moved.
> > Because of this, only devices which are permanently connected to
> > the PCI bus can use EA. A removable PCI card must not use EA.
> 
> As it is fixed, can you put IORESOURCE_PCI_FIXED in the res->flags?
> 
> Thanks
> 
> Yinghai

IORESOURCE_PCI_FIXED is set in pci_ea_set_flags():
 
+static unsigned long pci_ea_set_flags(struct pci_dev *dev, u8 prop)
+{
+   unsigned long flags = IORESOURCE_PCI_FIXED;
+
+

Which is called in pci_ea_read(), and passed to res->flags (if there aren't any 
errors):

+
+   /* Try to use primary properties, otherwise fall back to 
secondary */
+   flags = pci_ea_set_flags(dev, PCI_EA_PP(dw0));
+   if (!flags)
+   flags = pci_ea_set_flags(dev, PCI_EA_SP(dw0));
+

...

+   res->flags = flags;


-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

Add support for devices using Enhanced Allocation entries instead of BARs.
This patch allows the kernel to parse the EA Extended Capability structure
in PCI configspace and claim the BAR-equivalent resources.

Signed-off-by: Sean O. Stalley 
---
 drivers/pci/pci.c   | 174 
 drivers/pci/pci.h   |   1 +
 drivers/pci/probe.c |   3 +
 3 files changed, 178 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6a9a111..bdf5d37 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2148,6 +2148,180 @@ void pci_pm_init(struct pci_dev *dev)
}
 }
 
+static unsigned long pci_ea_set_flags(struct pci_dev *dev, u8 prop)
+{
+   unsigned long flags = IORESOURCE_PCI_FIXED;
+
+   switch (prop) {
+   case PCI_EA_P_MEM:
+   flags |= IORESOURCE_MEM;
+   break;
+   case PCI_EA_P_MEM_PREFETCH:
+   flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
+   break;
+   case PCI_EA_P_IO:
+   flags |= IORESOURCE_IO;
+   break;
+   default:
+   return 0;
+   }
+
+   return flags;
+}
+
+static struct resource *pci_ea_get_resource(struct pci_dev *dev, u8 bei)
+{
+   if (bei <= PCI_STD_RESOURCE_END)
+   return >resource[bei];
+   else if (bei == PCI_EA_BEI_ROM)
+   return >resource[PCI_ROM_RESOURCE];
+   else
+   return NULL;
+}
+
+/* Read an Enhanced Allocation (EA) entry */
+static int pci_ea_read(struct pci_dev *dev, int offset)
+{
+   struct resource *res;
+   int ent_offset = offset;
+   int ent_size;
+   resource_size_t start;
+   resource_size_t end;
+   unsigned long flags;
+   u32 dw0;
+   u32 base;
+   u32 max_offset;
+   bool support_64 = (sizeof(resource_size_t) >= 8);
+
+   pci_read_config_dword(dev, ent_offset, );
+   ent_offset += 4;
+
+   /* Entry size field indicates DWORDs after 1st */
+   ent_size = ((dw0 & PCI_EA_ES) + 1) << 2;
+
+   if (!(dw0 & PCI_EA_ENABLE)) /* Entry not enabled */
+   goto out;
+
+   res = pci_ea_get_resource(dev, PCI_EA_BEI(dw0));
+   if (!res) {
+   dev_err(>dev, "%s: Unsupported EA entry BEI\n", __func__);
+   goto out;
+   }
+
+   /* Try to use primary properties, otherwise fall back to secondary */
+   flags = pci_ea_set_flags(dev, PCI_EA_PP(dw0));
+   if (!flags)
+   flags = pci_ea_set_flags(dev, PCI_EA_SP(dw0));
+
+   if (!flags) {
+   dev_err(>dev, "%s: Entry EA properties not supported\n",
+   __func__);
+   goto out;
+   }
+
+   /* Read Base */
+   pci_read_config_dword(dev, ent_offset, );
+   start = (base & PCI_EA_FIELD_MASK);
+   ent_offset += 4;
+
+   /* Read MaxOffset */
+   pci_read_config_dword(dev, ent_offset, _offset);
+   ent_offset += 4;
+
+   /* Read Base MSBs (if 64-bit entry) */
+   if (base & PCI_EA_IS_64) {
+   u32 base_upper;
+
+   pci_read_config_dword(dev, ent_offset, _upper);
+   ent_offset += 4;
+
+   flags |= IORESOURCE_MEM_64;
+
+   /* entry starts above 32-bit boundary, can't use */
+   if (!support_64 && base_upper)
+   goto out;
+
+   if (support_64)
+   start |= ((u64)base_upper << 32);
+   }
+
+   dev_dbg(>dev, "%s: start = %pa\n", __func__, );
+
+   end = start + (max_offset | 0x03);
+
+   /* Read MaxOffset MSBs (if 64-bit entry) */
+   if (max_offset & PCI_EA_IS_64) {
+   u32 max_offset_upper;
+
+   pci_read_config_dword(dev, ent_offset, _offset_upper);
+   ent_offset += 4;
+
+   flags |= IORESOURCE_MEM_64;
+
+   /* entry too big, can't use */
+   if (!support_64 && max_offset_upper)
+   goto out;
+
+   if (support_64)
+   end += ((u64)max_offset_upper << 32);
+   }
+
+   dev_dbg(>dev, "%s: end = %pa\n", __func__, );
+
+   if (end < start) {
+   dev_err(>dev, "EA Entry crosses address boundary\n");
+   goto out;
+   }
+
+   if (ent_size != ent_offset - offset) {
+   dev_err(>dev, "EA entry size does not match length read\n"
+   "(Entry Size:%u Length Read:%u)\n",
+   ent_size, ent_offset - offset);
+   goto out;
+   }
+
+   res->name = pci_name(dev);
+   res->start = start;
+   res->end = end;
+   res->flags = flags;
+
+out:
+   return offset + ent_size;
+}
+
+/* Enhanced Allocation Initalization */
+void pci_ea_init(struct pci_dev *dev)
+{

[PATCH v2 0/2] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

PCI Enhanced Allocation is a new method of allocating MMIO & IO
resources for PCI devices & bridges. It can be used instead
of the traditional PCI method of using BARs.

EA entries are hardware-initialized to a fixed address.
Unlike BARs, regions described by EA are cannot be moved.
Because of this, only devices which are permanently connected to
the PCI bus can use EA. A removable PCI card must not use EA.

This patchset adds support for using EA entries instead of BARs
on Root Complex Integrated Endpoints.

The Enhanced Allocation ECN is publicly available here:
https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf


Changes from V1:
- Use generic PCI resource claim functions (instead of EA-specific 
functions)
- Only add support for RCiEPs (instead of all devices).
- Removed some debugging messages leftover from early testing.


Sean O. Stalley (2):
  PCI: Add Enhanced Allocation register entries
  PCI: Add support for Enhanced Allocation devices

 drivers/pci/pci.c | 174 ++
 drivers/pci/pci.h |   1 +
 drivers/pci/probe.c   |   3 +
 include/uapi/linux/pci_regs.h |  40 +-
 4 files changed, 217 insertions(+), 1 deletion(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/2] PCI: Add Enhanced Allocation register entries

[Sean O. Stalley]

Add registers defined in PCI-SIG's Enhanced allocation ECN.

Signed-off-by: Sean O. Stalley 
---
 include/uapi/linux/pci_regs.h | 40 +++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 413417f..084ce98 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -216,7 +216,8 @@
 #define  PCI_CAP_ID_MSIX   0x11/* MSI-X */
 #define  PCI_CAP_ID_SATA   0x12/* SATA Data/Index Conf. */
 #define  PCI_CAP_ID_AF 0x13/* PCI Advanced Features */
-#define  PCI_CAP_ID_MAXPCI_CAP_ID_AF
+#define  PCI_CAP_ID_EA 0x14/* PCI Enhanced Allocation */
+#define  PCI_CAP_ID_MAXPCI_CAP_ID_EA
 #define PCI_CAP_LIST_NEXT  1   /* Next capability in the list */
 #define PCI_CAP_FLAGS  2   /* Capability defined flags (16 bits) */
 #define PCI_CAP_SIZEOF 4
@@ -353,6 +354,43 @@
 #define  PCI_AF_STATUS_TP  0x01
 #define PCI_CAP_AF_SIZEOF  6   /* size of AF registers */
 
+/* PCI Enhanced Allocation registers */
+
+#define PCI_EA_NUM_ENT 2   /* Number of Capability Entries */
+#define PCI_EA_NUM_ENT_MASK0x3f/* Num Entries Mask */
+#define PCI_EA_FIRST_ENT   4   /* First EA Entry in List */
+#define PCI_EA_FIRST_ENT_BRIDGE8   /* First EA Entry for Bridges */
+#define PCI_EA_ES  0x7 /* Entry Size */
+#define PCI_EA_BEI(x)  (((x) >> 4) & 0xf) /* BAR Equivalent Indicator */
+/* 0-5 map to BARs 0-5 respectively */
+#define  PCI_EA_BEI_BRIDGE 6   /* Resource behind bridge */
+#define  PCI_EA_BEI_ENI7   /* Equivalent Not Indicated */
+#define  PCI_EA_BEI_ROM8   /* Expansion ROM */
+/* 9-14 map to VF BARs 0-5 respectively */
+#define  PCI_EA_BEI_RESERVED   15  /* Reserved - Treat like ENI */
+
+#define PCI_EA_PP(x)   (((x) >>  8) & 0xff)/* Primary Properties */
+#define PCI_EA_SP(x)   (((x) >> 16) & 0xff)/* Secondary Properties */
+#define  PCI_EA_P_MEM  0x00/* Non-Prefetch Memory */
+#define  PCI_EA_P_MEM_PREFETCH 0x01/* Prefetchable Memory */
+#define  PCI_EA_P_IO   0x02/* I/O Space */
+#define  PCI_EA_P_VIRT_MEM_PREFETCH0x03/* VF Prefetchable Memory */
+#define  PCI_EA_P_VIRT_MEM 0x04/* VF Non-Prefetch Memory */
+#define  PCI_EA_P_BRIDGE_MEM   0x05/* Bridge Non-Prefetch Memory */
+#define  PCI_EA_P_BRIDGE_MEM_PREFETCH  0x06/* Bridge Prefetchable Memory */
+#define  PCI_EA_P_BRIDGE_IO0x07/* Bridge I/O Space */
+/* 0x08-0xfc reserved */
+#define  PCI_EA_P_MEM_RESERVED 0xfd/* Reserved Memory */
+#define  PCI_EA_P_IO_RESERVED  0xfe/* Reserved I/O Space */
+#define  PCI_EA_P_UNAVAILABLE  0xff/* Entry Unavailable */
+#define PCI_EA_WRITEABLE   BIT(30) /* Writable, 1 = RW, 0 = HwInit */
+#define PCI_EA_ENABLE  BIT(31) /* Enable for this entry */
+#define PCI_EA_BASE4   /* Base Address Offset */
+#define PCI_EA_MAX_OFFSET  8   /* MaxOffset (resource length) */
+/* bit 0 is reserved */
+#define PCI_EA_IS_64   BIT(1)  /* 64-bit field flag */
+#define PCI_EA_FIELD_MASK  0xfffc  /* For Base & Max Offset */
+
 /* PCI-X registers (Type 0 (non-bridge) devices) */
 
 #define PCI_X_CMD  2   /* Modes & Features */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/2] PCI: Add Enhanced Allocation register entries

[Sean O. Stalley]

Add registers defined in PCI-SIG's Enhanced allocation ECN.

Signed-off-by: Sean O. Stalley <sean.stal...@intel.com>
---
 include/uapi/linux/pci_regs.h | 40 +++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 413417f..084ce98 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -216,7 +216,8 @@
 #define  PCI_CAP_ID_MSIX   0x11/* MSI-X */
 #define  PCI_CAP_ID_SATA   0x12/* SATA Data/Index Conf. */
 #define  PCI_CAP_ID_AF 0x13/* PCI Advanced Features */
-#define  PCI_CAP_ID_MAXPCI_CAP_ID_AF
+#define  PCI_CAP_ID_EA 0x14/* PCI Enhanced Allocation */
+#define  PCI_CAP_ID_MAXPCI_CAP_ID_EA
 #define PCI_CAP_LIST_NEXT  1   /* Next capability in the list */
 #define PCI_CAP_FLAGS  2   /* Capability defined flags (16 bits) */
 #define PCI_CAP_SIZEOF 4
@@ -353,6 +354,43 @@
 #define  PCI_AF_STATUS_TP  0x01
 #define PCI_CAP_AF_SIZEOF  6   /* size of AF registers */
 
+/* PCI Enhanced Allocation registers */
+
+#define PCI_EA_NUM_ENT 2   /* Number of Capability Entries */
+#define PCI_EA_NUM_ENT_MASK0x3f/* Num Entries Mask */
+#define PCI_EA_FIRST_ENT   4   /* First EA Entry in List */
+#define PCI_EA_FIRST_ENT_BRIDGE8   /* First EA Entry for Bridges */
+#define PCI_EA_ES  0x7 /* Entry Size */
+#define PCI_EA_BEI(x)  (((x) >> 4) & 0xf) /* BAR Equivalent Indicator */
+/* 0-5 map to BARs 0-5 respectively */
+#define  PCI_EA_BEI_BRIDGE 6   /* Resource behind bridge */
+#define  PCI_EA_BEI_ENI7   /* Equivalent Not Indicated */
+#define  PCI_EA_BEI_ROM8   /* Expansion ROM */
+/* 9-14 map to VF BARs 0-5 respectively */
+#define  PCI_EA_BEI_RESERVED   15  /* Reserved - Treat like ENI */
+
+#define PCI_EA_PP(x)   (((x) >>  8) & 0xff)/* Primary Properties */
+#define PCI_EA_SP(x)   (((x) >> 16) & 0xff)/* Secondary Properties */
+#define  PCI_EA_P_MEM  0x00/* Non-Prefetch Memory */
+#define  PCI_EA_P_MEM_PREFETCH 0x01/* Prefetchable Memory */
+#define  PCI_EA_P_IO   0x02/* I/O Space */
+#define  PCI_EA_P_VIRT_MEM_PREFETCH0x03/* VF Prefetchable Memory */
+#define  PCI_EA_P_VIRT_MEM 0x04/* VF Non-Prefetch Memory */
+#define  PCI_EA_P_BRIDGE_MEM   0x05/* Bridge Non-Prefetch Memory */
+#define  PCI_EA_P_BRIDGE_MEM_PREFETCH  0x06/* Bridge Prefetchable Memory */
+#define  PCI_EA_P_BRIDGE_IO0x07/* Bridge I/O Space */
+/* 0x08-0xfc reserved */
+#define  PCI_EA_P_MEM_RESERVED 0xfd/* Reserved Memory */
+#define  PCI_EA_P_IO_RESERVED  0xfe/* Reserved I/O Space */
+#define  PCI_EA_P_UNAVAILABLE  0xff/* Entry Unavailable */
+#define PCI_EA_WRITEABLE   BIT(30) /* Writable, 1 = RW, 0 = HwInit */
+#define PCI_EA_ENABLE  BIT(31) /* Enable for this entry */
+#define PCI_EA_BASE4   /* Base Address Offset */
+#define PCI_EA_MAX_OFFSET  8   /* MaxOffset (resource length) */
+/* bit 0 is reserved */
+#define PCI_EA_IS_64   BIT(1)  /* 64-bit field flag */
+#define PCI_EA_FIELD_MASK  0xfffc  /* For Base & Max Offset */
+
 /* PCI-X registers (Type 0 (non-bridge) devices) */
 
 #define PCI_X_CMD  2   /* Modes & Features */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

Add support for devices using Enhanced Allocation entries instead of BARs.
This patch allows the kernel to parse the EA Extended Capability structure
in PCI configspace and claim the BAR-equivalent resources.

Signed-off-by: Sean O. Stalley <sean.stal...@intel.com>
---
 drivers/pci/pci.c   | 174 
 drivers/pci/pci.h   |   1 +
 drivers/pci/probe.c |   3 +
 3 files changed, 178 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 6a9a111..bdf5d37 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2148,6 +2148,180 @@ void pci_pm_init(struct pci_dev *dev)
}
 }
 
+static unsigned long pci_ea_set_flags(struct pci_dev *dev, u8 prop)
+{
+   unsigned long flags = IORESOURCE_PCI_FIXED;
+
+   switch (prop) {
+   case PCI_EA_P_MEM:
+   flags |= IORESOURCE_MEM;
+   break;
+   case PCI_EA_P_MEM_PREFETCH:
+   flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
+   break;
+   case PCI_EA_P_IO:
+   flags |= IORESOURCE_IO;
+   break;
+   default:
+   return 0;
+   }
+
+   return flags;
+}
+
+static struct resource *pci_ea_get_resource(struct pci_dev *dev, u8 bei)
+{
+   if (bei <= PCI_STD_RESOURCE_END)
+   return >resource[bei];
+   else if (bei == PCI_EA_BEI_ROM)
+   return >resource[PCI_ROM_RESOURCE];
+   else
+   return NULL;
+}
+
+/* Read an Enhanced Allocation (EA) entry */
+static int pci_ea_read(struct pci_dev *dev, int offset)
+{
+   struct resource *res;
+   int ent_offset = offset;
+   int ent_size;
+   resource_size_t start;
+   resource_size_t end;
+   unsigned long flags;
+   u32 dw0;
+   u32 base;
+   u32 max_offset;
+   bool support_64 = (sizeof(resource_size_t) >= 8);
+
+   pci_read_config_dword(dev, ent_offset, );
+   ent_offset += 4;
+
+   /* Entry size field indicates DWORDs after 1st */
+   ent_size = ((dw0 & PCI_EA_ES) + 1) << 2;
+
+   if (!(dw0 & PCI_EA_ENABLE)) /* Entry not enabled */
+   goto out;
+
+   res = pci_ea_get_resource(dev, PCI_EA_BEI(dw0));
+   if (!res) {
+   dev_err(>dev, "%s: Unsupported EA entry BEI\n", __func__);
+   goto out;
+   }
+
+   /* Try to use primary properties, otherwise fall back to secondary */
+   flags = pci_ea_set_flags(dev, PCI_EA_PP(dw0));
+   if (!flags)
+   flags = pci_ea_set_flags(dev, PCI_EA_SP(dw0));
+
+   if (!flags) {
+   dev_err(>dev, "%s: Entry EA properties not supported\n",
+   __func__);
+   goto out;
+   }
+
+   /* Read Base */
+   pci_read_config_dword(dev, ent_offset, );
+   start = (base & PCI_EA_FIELD_MASK);
+   ent_offset += 4;
+
+   /* Read MaxOffset */
+   pci_read_config_dword(dev, ent_offset, _offset);
+   ent_offset += 4;
+
+   /* Read Base MSBs (if 64-bit entry) */
+   if (base & PCI_EA_IS_64) {
+   u32 base_upper;
+
+   pci_read_config_dword(dev, ent_offset, _upper);
+   ent_offset += 4;
+
+   flags |= IORESOURCE_MEM_64;
+
+   /* entry starts above 32-bit boundary, can't use */
+   if (!support_64 && base_upper)
+   goto out;
+
+   if (support_64)
+   start |= ((u64)base_upper << 32);
+   }
+
+   dev_dbg(>dev, "%s: start = %pa\n", __func__, );
+
+   end = start + (max_offset | 0x03);
+
+   /* Read MaxOffset MSBs (if 64-bit entry) */
+   if (max_offset & PCI_EA_IS_64) {
+   u32 max_offset_upper;
+
+   pci_read_config_dword(dev, ent_offset, _offset_upper);
+   ent_offset += 4;
+
+   flags |= IORESOURCE_MEM_64;
+
+   /* entry too big, can't use */
+   if (!support_64 && max_offset_upper)
+   goto out;
+
+   if (support_64)
+   end += ((u64)max_offset_upper << 32);
+   }
+
+   dev_dbg(>dev, "%s: end = %pa\n", __func__, );
+
+   if (end < start) {
+   dev_err(>dev, "EA Entry crosses address boundary\n");
+   goto out;
+   }
+
+   if (ent_size != ent_offset - offset) {
+   dev_err(>dev, "EA entry size does not match length read\n"
+   "(Entry Size:%u Length Read:%u)\n",
+   ent_size, ent_offset - offset);
+   goto out;
+   }
+
+   res->name = pci_name(dev);
+   res->start = start;
+   res->end = end;
+   res->flags = flags;
+
+out:
+   return offset + ent_size;
+}
+
+/* Enhanced Allocation Initalization */
+void pci_ea_init(str

[PATCH v2 0/2] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

PCI Enhanced Allocation is a new method of allocating MMIO & IO
resources for PCI devices & bridges. It can be used instead
of the traditional PCI method of using BARs.

EA entries are hardware-initialized to a fixed address.
Unlike BARs, regions described by EA are cannot be moved.
Because of this, only devices which are permanently connected to
the PCI bus can use EA. A removable PCI card must not use EA.

This patchset adds support for using EA entries instead of BARs
on Root Complex Integrated Endpoints.

The Enhanced Allocation ECN is publicly available here:
https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf


Changes from V1:
- Use generic PCI resource claim functions (instead of EA-specific 
functions)
- Only add support for RCiEPs (instead of all devices).
- Removed some debugging messages leftover from early testing.


Sean O. Stalley (2):
  PCI: Add Enhanced Allocation register entries
  PCI: Add support for Enhanced Allocation devices

 drivers/pci/pci.c | 174 ++
 drivers/pci/pci.h |   1 +
 drivers/pci/probe.c   |   3 +
 include/uapi/linux/pci_regs.h |  40 +-
 4 files changed, 217 insertions(+), 1 deletion(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/2] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

On Wed, Sep 23, 2015 at 04:26:56PM -0700, Yinghai Lu wrote:
> On Wed, Sep 23, 2015 at 3:27 PM, Sean O. Stalley <sean.stal...@intel.com> 
> wrote:
> > PCI Enhanced Allocation is a new method of allocating MMIO & IO
> > resources for PCI devices & bridges. It can be used instead
> > of the traditional PCI method of using BARs.
> >
> > EA entries are hardware-initialized to a fixed address.
> > Unlike BARs, regions described by EA are cannot be moved.
> > Because of this, only devices which are permanently connected to
> > the PCI bus can use EA. A removable PCI card must not use EA.
> 
> As it is fixed, can you put IORESOURCE_PCI_FIXED in the res->flags?
> 
> Thanks
> 
> Yinghai

IORESOURCE_PCI_FIXED is set in pci_ea_set_flags():
 
+static unsigned long pci_ea_set_flags(struct pci_dev *dev, u8 prop)
+{
+   unsigned long flags = IORESOURCE_PCI_FIXED;
+
+

Which is called in pci_ea_read(), and passed to res->flags (if there aren't any 
errors):

+
+   /* Try to use primary properties, otherwise fall back to 
secondary */
+   flags = pci_ea_set_flags(dev, PCI_EA_PP(dw0));
+   if (!flags)
+   flags = pci_ea_set_flags(dev, PCI_EA_SP(dw0));
+

...

+   res->flags = flags;


-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

On Thu, Sep 03, 2015 at 09:46:54AM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 02, 2015 at 05:29:38PM -0700, Sean O. Stalley wrote:
> > On Wed, Sep 02, 2015 at 04:21:59PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 02, 2015 at 01:01:27PM -0700, Sean O. Stalley wrote:
> > > > On Wed, Sep 02, 2015 at 02:25:50PM -0500, Bjorn Helgaas wrote:
> > > > > On Wed, Sep 2, 2015 at 12:46 PM, Sean O. Stalley 
> > > > >  wrote:
> > > > > 
> > > > > > Would it be better to modify pci_claim_resource() to support EA 
> > > > > > instead of adding pci_ea_claim_resource()?
> > > > > > That way, EA entries would be claimed at the same time as 
> > > > > > traditional BARs.
> > > > > 
> > > > > Yes, I think so.
> > > > 
> > > > Ok, I'll make it work this way in the next patchset.
> > > > 
> > > > > Why wouldn't pci_claim_resource() work as-is for EA?  I see that
> > > > > pci_ea_get_parent_resource() defaults to iomem_resource or
> > > > > ioport_resource if we don't find a parent, but I don't understand why
> > > > > that's necessary.
> > > > 
> > > > EA resources may (or may not) be in the parent's range[1].
> > > > If the parent doesn't describe this range, we want to default to the 
> > > > top-level resource.
> > > > Other than that case, I think pci_claim_resource would work as-is.
> > > > 
> > > > -Sean
> > > > 
> > > > [1] From the EA ECN:
> > > > For a bridge function that is permitted to implement EA based on the 
> > > > rules above, it is
> > > > permitted, but not required, for the bridge function to use EA 
> > > > mechanisms to indicate
> > > > resource ranges that are located behind the bridge Function (see 
> > > > Section 6.9.1.2).
> > > 
> > > [BTW, in EA ECN 23_Oct_2014_Final, this text is in sec 6.9, not 6.9.1.2]
> > > 
> > > I agree that it implies EA resources need not be in the parent's *EA*
> > > range.  But I would read it as saying "a bridge can use either the
> > > usual window registers or EA to indicate resources forwarded
> > > downstream."
> > > 
> > > What happens in the following case?
> > > 
> > >   00:00.0: PCI bridge to [bus 01]
> > >   00:00.0:   bridge window [mem 0x8000-0x8fff]
> > >   01:00.0: EA 0: [mem 0x9000-0x9000]
> > >
> > > The 00:00.0 bridge knows nothing about EA.  The 01:00.0 EA device has
> > > a fixed region at 0x9000.  The ECN says:
> > > 
> > >   System firmware/software must comprehend that such bridge functions
> > >   [those that are permitted to implement EA] are not required to
> > >   indicate inclusively all resources behind the bridge, and as a
> > >   result system firmware/software must make a complete search of all
> > >   functions behind the bridge to comprehend the resources used by
> > >   those functions.
> > 
> > The intention of this line was to indicate that EA regions are not required
> > to be inside of the Base+Limit window.
> 
> It would be a lot nicer if the terminology here built on terminology
> used in the original specs.  The P2P Bridge spec defines rules for
> when a bridge forwards transactions between its primary and secondary
> interfaces using Command register Enable bits and Base and Limit
> registers.  It doesn't say anything about "indicating resources behind
> the bridge."

Thanks for the feedback. I will forward it on the spec-writer.

> > If an EA device is connected below a bridge, that bridge must be aware of 
> > EA.
> > It is assumed that the bridge is aware of the fixed EA regions below it,
> > so system software doesn't need to program the window to include them.
> 
> Is the requirement that every bridge upstream of an EA device must be
> aware of EA in the ECN somewhere?  What does it even mean for a bridge
> to be "aware of EA"?  That it has an EA Capability?

Sorry, poor choice of words on my part. Yes, it must be an EA bridge.

Also, I should point out that this patchset doesn't add support for EA bridges.
It only adds support for EA endpoints that are using an EA entries instead of 
BARs.

> The EA ECN doesn't change anything in the P2P bridge spec, so a bridge
> that forwards EA regions not described by its Base/Limit registers
> sounds like it would be non-conforming.

Your right, It it seems like there would need to be a change to 

Re: [PATCH 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

On Thu, Sep 03, 2015 at 09:46:54AM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 02, 2015 at 05:29:38PM -0700, Sean O. Stalley wrote:
> > On Wed, Sep 02, 2015 at 04:21:59PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 02, 2015 at 01:01:27PM -0700, Sean O. Stalley wrote:
> > > > On Wed, Sep 02, 2015 at 02:25:50PM -0500, Bjorn Helgaas wrote:
> > > > > On Wed, Sep 2, 2015 at 12:46 PM, Sean O. Stalley 
> > > > > <sean.stal...@intel.com> wrote:
> > > > > 
> > > > > > Would it be better to modify pci_claim_resource() to support EA 
> > > > > > instead of adding pci_ea_claim_resource()?
> > > > > > That way, EA entries would be claimed at the same time as 
> > > > > > traditional BARs.
> > > > > 
> > > > > Yes, I think so.
> > > > 
> > > > Ok, I'll make it work this way in the next patchset.
> > > > 
> > > > > Why wouldn't pci_claim_resource() work as-is for EA?  I see that
> > > > > pci_ea_get_parent_resource() defaults to iomem_resource or
> > > > > ioport_resource if we don't find a parent, but I don't understand why
> > > > > that's necessary.
> > > > 
> > > > EA resources may (or may not) be in the parent's range[1].
> > > > If the parent doesn't describe this range, we want to default to the 
> > > > top-level resource.
> > > > Other than that case, I think pci_claim_resource would work as-is.
> > > > 
> > > > -Sean
> > > > 
> > > > [1] From the EA ECN:
> > > > For a bridge function that is permitted to implement EA based on the 
> > > > rules above, it is
> > > > permitted, but not required, for the bridge function to use EA 
> > > > mechanisms to indicate
> > > > resource ranges that are located behind the bridge Function (see 
> > > > Section 6.9.1.2).
> > > 
> > > [BTW, in EA ECN 23_Oct_2014_Final, this text is in sec 6.9, not 6.9.1.2]
> > > 
> > > I agree that it implies EA resources need not be in the parent's *EA*
> > > range.  But I would read it as saying "a bridge can use either the
> > > usual window registers or EA to indicate resources forwarded
> > > downstream."
> > > 
> > > What happens in the following case?
> > > 
> > >   00:00.0: PCI bridge to [bus 01]
> > >   00:00.0:   bridge window [mem 0x8000-0x8fff]
> > >   01:00.0: EA 0: [mem 0x9000-0x9000]
> > >
> > > The 00:00.0 bridge knows nothing about EA.  The 01:00.0 EA device has
> > > a fixed region at 0x9000.  The ECN says:
> > > 
> > >   System firmware/software must comprehend that such bridge functions
> > >   [those that are permitted to implement EA] are not required to
> > >   indicate inclusively all resources behind the bridge, and as a
> > >   result system firmware/software must make a complete search of all
> > >   functions behind the bridge to comprehend the resources used by
> > >   those functions.
> > 
> > The intention of this line was to indicate that EA regions are not required
> > to be inside of the Base+Limit window.
> 
> It would be a lot nicer if the terminology here built on terminology
> used in the original specs.  The P2P Bridge spec defines rules for
> when a bridge forwards transactions between its primary and secondary
> interfaces using Command register Enable bits and Base and Limit
> registers.  It doesn't say anything about "indicating resources behind
> the bridge."

Thanks for the feedback. I will forward it on the spec-writer.

> > If an EA device is connected below a bridge, that bridge must be aware of 
> > EA.
> > It is assumed that the bridge is aware of the fixed EA regions below it,
> > so system software doesn't need to program the window to include them.
> 
> Is the requirement that every bridge upstream of an EA device must be
> aware of EA in the ECN somewhere?  What does it even mean for a bridge
> to be "aware of EA"?  That it has an EA Capability?

Sorry, poor choice of words on my part. Yes, it must be an EA bridge.

Also, I should point out that this patchset doesn't add support for EA bridges.
It only adds support for EA endpoints that are using an EA entries instead of 
BARs.

> The EA ECN doesn't change anything in the P2P bridge spec, so a bridge
> that forwards EA regions not described by its Base/Limit registers
> sounds like it would be non-conforming.

Your right, It it seems like there woul

Re: [PATCH 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

On Wed, Sep 02, 2015 at 04:21:59PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 02, 2015 at 01:01:27PM -0700, Sean O. Stalley wrote:
> > On Wed, Sep 02, 2015 at 02:25:50PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 2, 2015 at 12:46 PM, Sean O. Stalley  
> > > wrote:
> > > 
> > > > Would it be better to modify pci_claim_resource() to support EA instead 
> > > > of adding pci_ea_claim_resource()?
> > > > That way, EA entries would be claimed at the same time as traditional 
> > > > BARs.
> > > 
> > > Yes, I think so.
> > 
> > Ok, I'll make it work this way in the next patchset.
> > 
> > > Why wouldn't pci_claim_resource() work as-is for EA?  I see that
> > > pci_ea_get_parent_resource() defaults to iomem_resource or
> > > ioport_resource if we don't find a parent, but I don't understand why
> > > that's necessary.
> > 
> > EA resources may (or may not) be in the parent's range[1].
> > If the parent doesn't describe this range, we want to default to the 
> > top-level resource.
> > Other than that case, I think pci_claim_resource would work as-is.
> > 
> > -Sean
> > 
> > [1] From the EA ECN:
> > For a bridge function that is permitted to implement EA based on the rules 
> > above, it is
> > permitted, but not required, for the bridge function to use EA mechanisms 
> > to indicate
> > resource ranges that are located behind the bridge Function (see Section 
> > 6.9.1.2).
> 
> [BTW, in EA ECN 23_Oct_2014_Final, this text is in sec 6.9, not 6.9.1.2]
> 
> I agree that it implies EA resources need not be in the parent's *EA*
> range.  But I would read it as saying "a bridge can use either the
> usual window registers or EA to indicate resources forwarded
> downstream."
> 
> What happens in the following case?
> 
>   00:00.0: PCI bridge to [bus 01]
>   00:00.0:   bridge window [mem 0x8000-0x8fff]
>   01:00.0: EA 0: [mem 0x9000-0x9000]
>
> The 00:00.0 bridge knows nothing about EA.  The 01:00.0 EA device has
> a fixed region at 0x9000.  The ECN says:
> 
>   System firmware/software must comprehend that such bridge functions
>   [those that are permitted to implement EA] are not required to
>   indicate inclusively all resources behind the bridge, and as a
>   result system firmware/software must make a complete search of all
>   functions behind the bridge to comprehend the resources used by
>   those functions.
> 

The intention of this line was to indicate that EA regions are not required
to be inside of the Base+Limit window.

If an EA device is connected below a bridge, that bridge must be aware of EA.
It is assumed that the bridge is aware of the fixed EA regions below it,
so system software doesn't need to program the window to include them.

This is part of the reason why EA devices must be permanently connected
(to make sure it doesn't end up behind an old bridge).
Re-reading the spec, I can see that this requirement isn't explicitly stated.

> A bridge was never required to indicate, e.g., via its window
> registers, anything about the resources behind it.  Software always
> had to search behind the bridge and look at all the downstream BARs.
> What's new here is that software now has to look for downstream EA
> entries in addition to BARs, and the EA entries are at fixed
> addresses.
> 
> My question is what the implication is for address routing performed
> by the bridge.  The EA ECN doesn't mention any changes there, so I
> assume it is software's responsibility to reprogram the 00:00.0 mem
> window so it includes [mem 0x9000-0x9000].

The Base+Limit window is not required to include EA regions.
In the example shown in in Figure 6-1, the bridge above Bus N [...]
is permitted to not indicate the resources used by the two functions
in “Si component C”

Before, all the BAR regions would be inside the window range.
The Base+Limit "indicated" the Range of all the BARs behind the bridge.
Once the window was set, system software could avoid an address collision
with every device on the bus by avoiding the window.

BAR-equivalent EA regions aren't required to be inside the Base+Limit window,
which is why System firmware/software must search all the functions behind a bus
to avoid address collisions.

> If software does have to reprogram that window, the normal
> pci_claim_resource() should work.  If it doesn't have to reprogram the
> window, and there's some magical way for 01:00.0 to work even though
> we don't route address space to it, I suspect we'll need significantly
> more changes than just pci_ea_claim_resource(), because then 00:00.0
> is really not a PCI bridge any more.
> 
> Bjorn

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

On Wed, Sep 02, 2015 at 02:25:50PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 2, 2015 at 12:46 PM, Sean O. Stalley  
> wrote:
> 
> > Would it be better to modify pci_claim_resource() to support EA instead of 
> > adding pci_ea_claim_resource()?
> > That way, EA entries would be claimed at the same time as traditional BARs.
> 
> Yes, I think so.

Ok, I'll make it work this way in the next patchset.

> Why wouldn't pci_claim_resource() work as-is for EA?  I see that
> pci_ea_get_parent_resource() defaults to iomem_resource or
> ioport_resource if we don't find a parent, but I don't understand why
> that's necessary.

EA resources may (or may not) be in the parent's range[1].
If the parent doesn't describe this range, we want to default to the top-level 
resource.
Other than that case, I think pci_claim_resource would work as-is.

-Sean

[1] From the EA ECN:
For a bridge function that is permitted to implement EA based on the rules 
above, it is
permitted, but not required, for the bridge function to use EA mechanisms to 
indicate
resource ranges that are located behind the bridge Function (see Section 
6.9.1.2).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

Thanks for taking a look Yinghai.

On Tue, Sep 01, 2015 at 04:14:08PM -0700, Yinghai Lu wrote:
> On Thu, Aug 20, 2015 at 9:59 AM, Sean O. Stalley  
> wrote:
> > Add support for devices using Enhanced Allocation entries instead of BARs.
> > This patch allows the kernel to parse the EA Extended Capability structure
> > in PCI configspace and claim the BAR-equivalent resources.
> >
> > Signed-off-by: Sean O. Stalley 
> > ---
> >  drivers/pci/pci.c   | 219 
> > 
> >  drivers/pci/pci.h   |   1 +
> >  drivers/pci/probe.c |   3 +
> >  3 files changed, 223 insertions(+)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 0008c95..c8217a8 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> ...
> > +
> > +/* Read an Enhanced Allocation (EA) entry */
> > +static int pci_ea_read(struct pci_dev *dev, int offset)
> > +{
> ...
> > +   res->name = pci_name(dev);
> > +   res->start = start;
> > +   res->end = end;
> > +   res->flags = flags;
> > +
> > +   pci_ea_claim_resource(dev, res);
> > +
> > +out:
> > +   return offset + ent_size;
> > +}
> > +
> > +/* Enhanced Allocation Initalization */
> > +void pci_ea_init(struct pci_dev *dev)
> > +{
> ...
> > +
> > +   for (i = 0; i < num_ent; ++i) {
> > +   /* parse each EA entry */
> > +   dev_dbg(>dev, "%s: parsing entry %i...\n", __func__, 
> > i);
> > +   offset = pci_ea_read(dev, offset);
> > +   }
> > +}
> > +
> >  static void pci_add_saved_cap(struct pci_dev *pci_dev,
> > struct pci_cap_saved_state *new_cap)
> >  {
> 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index cefd636..4cadf35 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1522,6 +1522,9 @@ static struct pci_dev *pci_scan_device(struct pci_bus 
> > *bus, int devfn)
> >
> >  static void pci_init_capabilities(struct pci_dev *dev)
> >  {
> > +   /* Enhanced Allocation */
> > +   pci_ea_init(dev);
> > +
> > /* MSI/MSI-X list */
> > pci_msi_init_pci_dev(dev);
> >
> 
> Should not call pci_ea_claim_resource() that early.

Out of curiosity, why shouldn't resources be claimed that early?
EA resources are fixed by hardware. They are always there & will never move.

> 
> For x86 and other arches, we call
> pcibios_resource_survey/pcibios_allocate_bus_resouce/pcibios_allocate_resources
> quite late.

Would it be better to modify pci_claim_resource() to support EA instead of 
adding pci_ea_claim_resource()?
That way, EA entries would be claimed at the same time as traditional BARs.

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

Thanks for taking a look Yinghai.

On Tue, Sep 01, 2015 at 04:14:08PM -0700, Yinghai Lu wrote:
> On Thu, Aug 20, 2015 at 9:59 AM, Sean O. Stalley <sean.stal...@intel.com> 
> wrote:
> > Add support for devices using Enhanced Allocation entries instead of BARs.
> > This patch allows the kernel to parse the EA Extended Capability structure
> > in PCI configspace and claim the BAR-equivalent resources.
> >
> > Signed-off-by: Sean O. Stalley <sean.stal...@intel.com>
> > ---
> >  drivers/pci/pci.c   | 219 
> > 
> >  drivers/pci/pci.h   |   1 +
> >  drivers/pci/probe.c |   3 +
> >  3 files changed, 223 insertions(+)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 0008c95..c8217a8 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> ...
> > +
> > +/* Read an Enhanced Allocation (EA) entry */
> > +static int pci_ea_read(struct pci_dev *dev, int offset)
> > +{
> ...
> > +   res->name = pci_name(dev);
> > +   res->start = start;
> > +   res->end = end;
> > +   res->flags = flags;
> > +
> > +   pci_ea_claim_resource(dev, res);
> > +
> > +out:
> > +   return offset + ent_size;
> > +}
> > +
> > +/* Enhanced Allocation Initalization */
> > +void pci_ea_init(struct pci_dev *dev)
> > +{
> ...
> > +
> > +   for (i = 0; i < num_ent; ++i) {
> > +   /* parse each EA entry */
> > +   dev_dbg(>dev, "%s: parsing entry %i...\n", __func__, 
> > i);
> > +   offset = pci_ea_read(dev, offset);
> > +   }
> > +}
> > +
> >  static void pci_add_saved_cap(struct pci_dev *pci_dev,
> > struct pci_cap_saved_state *new_cap)
> >  {
> 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index cefd636..4cadf35 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1522,6 +1522,9 @@ static struct pci_dev *pci_scan_device(struct pci_bus 
> > *bus, int devfn)
> >
> >  static void pci_init_capabilities(struct pci_dev *dev)
> >  {
> > +   /* Enhanced Allocation */
> > +   pci_ea_init(dev);
> > +
> > /* MSI/MSI-X list */
> > pci_msi_init_pci_dev(dev);
> >
> 
> Should not call pci_ea_claim_resource() that early.

Out of curiosity, why shouldn't resources be claimed that early?
EA resources are fixed by hardware. They are always there & will never move.

> 
> For x86 and other arches, we call
> pcibios_resource_survey/pcibios_allocate_bus_resouce/pcibios_allocate_resources
> quite late.

Would it be better to modify pci_claim_resource() to support EA instead of 
adding pci_ea_claim_resource()?
That way, EA entries would be claimed at the same time as traditional BARs.

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

On Wed, Sep 02, 2015 at 04:21:59PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 02, 2015 at 01:01:27PM -0700, Sean O. Stalley wrote:
> > On Wed, Sep 02, 2015 at 02:25:50PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 2, 2015 at 12:46 PM, Sean O. Stalley <sean.stal...@intel.com> 
> > > wrote:
> > > 
> > > > Would it be better to modify pci_claim_resource() to support EA instead 
> > > > of adding pci_ea_claim_resource()?
> > > > That way, EA entries would be claimed at the same time as traditional 
> > > > BARs.
> > > 
> > > Yes, I think so.
> > 
> > Ok, I'll make it work this way in the next patchset.
> > 
> > > Why wouldn't pci_claim_resource() work as-is for EA?  I see that
> > > pci_ea_get_parent_resource() defaults to iomem_resource or
> > > ioport_resource if we don't find a parent, but I don't understand why
> > > that's necessary.
> > 
> > EA resources may (or may not) be in the parent's range[1].
> > If the parent doesn't describe this range, we want to default to the 
> > top-level resource.
> > Other than that case, I think pci_claim_resource would work as-is.
> > 
> > -Sean
> > 
> > [1] From the EA ECN:
> > For a bridge function that is permitted to implement EA based on the rules 
> > above, it is
> > permitted, but not required, for the bridge function to use EA mechanisms 
> > to indicate
> > resource ranges that are located behind the bridge Function (see Section 
> > 6.9.1.2).
> 
> [BTW, in EA ECN 23_Oct_2014_Final, this text is in sec 6.9, not 6.9.1.2]
> 
> I agree that it implies EA resources need not be in the parent's *EA*
> range.  But I would read it as saying "a bridge can use either the
> usual window registers or EA to indicate resources forwarded
> downstream."
> 
> What happens in the following case?
> 
>   00:00.0: PCI bridge to [bus 01]
>   00:00.0:   bridge window [mem 0x8000-0x8fff]
>   01:00.0: EA 0: [mem 0x9000-0x9000]
>
> The 00:00.0 bridge knows nothing about EA.  The 01:00.0 EA device has
> a fixed region at 0x9000.  The ECN says:
> 
>   System firmware/software must comprehend that such bridge functions
>   [those that are permitted to implement EA] are not required to
>   indicate inclusively all resources behind the bridge, and as a
>   result system firmware/software must make a complete search of all
>   functions behind the bridge to comprehend the resources used by
>   those functions.
> 

The intention of this line was to indicate that EA regions are not required
to be inside of the Base+Limit window.

If an EA device is connected below a bridge, that bridge must be aware of EA.
It is assumed that the bridge is aware of the fixed EA regions below it,
so system software doesn't need to program the window to include them.

This is part of the reason why EA devices must be permanently connected
(to make sure it doesn't end up behind an old bridge).
Re-reading the spec, I can see that this requirement isn't explicitly stated.

> A bridge was never required to indicate, e.g., via its window
> registers, anything about the resources behind it.  Software always
> had to search behind the bridge and look at all the downstream BARs.
> What's new here is that software now has to look for downstream EA
> entries in addition to BARs, and the EA entries are at fixed
> addresses.
> 
> My question is what the implication is for address routing performed
> by the bridge.  The EA ECN doesn't mention any changes there, so I
> assume it is software's responsibility to reprogram the 00:00.0 mem
> window so it includes [mem 0x9000-0x9000].

The Base+Limit window is not required to include EA regions.
In the example shown in in Figure 6-1, the bridge above Bus N [...]
is permitted to not indicate the resources used by the two functions
in “Si component C”

Before, all the BAR regions would be inside the window range.
The Base+Limit "indicated" the Range of all the BARs behind the bridge.
Once the window was set, system software could avoid an address collision
with every device on the bus by avoiding the window.

BAR-equivalent EA regions aren't required to be inside the Base+Limit window,
which is why System firmware/software must search all the functions behind a bus
to avoid address collisions.

> If software does have to reprogram that window, the normal
> pci_claim_resource() should work.  If it doesn't have to reprogram the
> window, and there's some magical way for 01:00.0 to work even though
> we don't route address space to it, I suspect we'll need significantly
> more changes than just pci_ea_claim_resource(), because then 00:00.0
> is really not a PCI bridge any more.
> 
> Bjorn

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

On Wed, Sep 02, 2015 at 02:25:50PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 2, 2015 at 12:46 PM, Sean O. Stalley <sean.stal...@intel.com> 
> wrote:
> 
> > Would it be better to modify pci_claim_resource() to support EA instead of 
> > adding pci_ea_claim_resource()?
> > That way, EA entries would be claimed at the same time as traditional BARs.
> 
> Yes, I think so.

Ok, I'll make it work this way in the next patchset.

> Why wouldn't pci_claim_resource() work as-is for EA?  I see that
> pci_ea_get_parent_resource() defaults to iomem_resource or
> ioport_resource if we don't find a parent, but I don't understand why
> that's necessary.

EA resources may (or may not) be in the parent's range[1].
If the parent doesn't describe this range, we want to default to the top-level 
resource.
Other than that case, I think pci_claim_resource would work as-is.

-Sean

[1] From the EA ECN:
For a bridge function that is permitted to implement EA based on the rules 
above, it is
permitted, but not required, for the bridge function to use EA mechanisms to 
indicate
resource ranges that are located behind the bridge Function (see Section 
6.9.1.2).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/2] PCI: Add support for PCI Enhanced Allocation "BARs"

[Sean O. Stalley]

PCI Enhanced Allocation is a new method of allocating MMIO & IO
resources for PCI devices & bridges. It can be used instead
of the traditional PCI method of using BARs.

EA entries are hardware-initialized to a fixed address.
Unlike BARs, regions described by EA are cannot be moved.
Because of this, only devices which are permanently connected to
the PCI bus can use EA. A removable PCI card must not use EA.

This patchset adds support for using EA entries instead of BARs.

The Enhanced Allocation ECN is publicly available here:
https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf

Sean O. Stalley (2):
  PCI: Add Enhanced Allocation register entries
  PCI: Add parsing of Enhanced Allocation entries

 drivers/pci/pci.c | 219 ++
 drivers/pci/pci.h |   1 +
 drivers/pci/probe.c   |   3 +
 include/uapi/linux/pci_regs.h |  40 +++-
 4 files changed, 262 insertions(+), 1 deletion(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

Add support for devices using Enhanced Allocation entries instead of BARs.
This patch allows the kernel to parse the EA Extended Capability structure
in PCI configspace and claim the BAR-equivalent resources.

Signed-off-by: Sean O. Stalley 
---
 drivers/pci/pci.c   | 219 
 drivers/pci/pci.h   |   1 +
 drivers/pci/probe.c |   3 +
 3 files changed, 223 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0008c95..c8217a8 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2134,6 +2134,225 @@ void pci_pm_init(struct pci_dev *dev)
}
 }
 
+static unsigned long pci_ea_set_flags(struct pci_dev *dev, u8 prop)
+{
+   unsigned long flags = IORESOURCE_PCI_FIXED;
+
+   switch (prop) {
+   case PCI_EA_P_MEM:
+   flags |= IORESOURCE_MEM;
+   break;
+   case PCI_EA_P_MEM_PREFETCH:
+   flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
+   break;
+   case PCI_EA_P_IO:
+   flags |= IORESOURCE_IO;
+   break;
+   default:
+   dev_warn(>dev, "%s: Property type %x not supported\n",
+   __func__, prop);
+   return 0;
+   }
+
+   return flags;
+}
+
+static struct resource *pci_ea_get_parent_resource(struct pci_dev *dev,
+   struct resource *res)
+{
+   struct resource *parent;
+
+   parent = pci_find_parent_resource(dev, res);
+   if (parent)
+   return parent;
+
+   /* for resources not claimed by a bridge */
+   if (res->flags & IORESOURCE_MEM)
+   return _resource;
+
+   if (res->flags & IORESOURCE_IO)
+   return _resource;
+
+   return NULL;
+}
+
+/* claim the memory for this device in the proper location */
+static void pci_ea_claim_resource(struct pci_dev *dev, struct resource *res)
+{
+   struct resource *parent;
+   struct resource *conflict;
+
+   parent = pci_ea_get_parent_resource(dev, res);
+   if (!parent) {
+   dev_warn(>dev, "can't find parent resource for EA entry %s 
%pR\n",
+   res->name, res);
+   return;
+   }
+
+   /* claim the appropriate resource */
+   conflict = request_resource_conflict(parent, res);
+   if (conflict) {
+   dev_warn(>dev, "can't claim EA entry %s %pR: address 
conflict with %s %pR\n",
+res->name, res, conflict->name, conflict);
+   }
+}
+
+static struct resource *pci_ea_get_resource(struct pci_dev *dev, u8 bei)
+{
+   if (bei <= PCI_STD_RESOURCE_END)
+   return >resource[bei];
+   else if (bei == PCI_EA_BEI_ROM)
+   return >resource[PCI_ROM_RESOURCE];
+   else
+   return NULL;
+}
+
+/* Read an Enhanced Allocation (EA) entry */
+static int pci_ea_read(struct pci_dev *dev, int offset)
+{
+   struct resource *res;
+   int ent_offset = offset;
+   int ent_size;
+   resource_size_t start;
+   resource_size_t end;
+   unsigned long flags;
+   u32 dw0;
+   u32 base;
+   u32 max_offset;
+   bool support_64 = (sizeof(resource_size_t) >= 8);
+
+   pci_read_config_dword(dev, ent_offset, );
+   ent_offset += 4;
+
+   /* Entry size field indicates DWORDs after 1st */
+   ent_size = ((dw0 & PCI_EA_ES) + 1) << 2;
+
+   if (!(dw0 & PCI_EA_ENABLE)) {
+   dev_err(>dev, "%s: Entry not enabled\n", __func__);
+   goto out;
+   }
+
+   res = pci_ea_get_resource(dev, PCI_EA_BEI(dw0));
+   if (!res) {
+   dev_err(>dev, "%s: Unsupported EA entry BEI\n", __func__);
+   goto out;
+   }
+
+   flags = pci_ea_set_flags(dev, PCI_EA_PP(dw0));
+   if (!flags)
+   flags = pci_ea_set_flags(dev, PCI_EA_SP(dw0));
+   if (!flags) {
+   dev_err(>dev, "%s: Entry EA properties not supported\n",
+   __func__);
+   goto out;
+   }
+
+   /* Read Base */
+   pci_read_config_dword(dev, ent_offset, );
+   start = (base & PCI_EA_FIELD_MASK);
+   ent_offset += 4;
+
+   /* Read MaxOffset */
+   pci_read_config_dword(dev, ent_offset, _offset);
+   ent_offset += 4;
+
+   /* Read Base MSBs (if 64-bit entry) */
+   if (base & PCI_EA_IS_64) {
+   u32 base_upper;
+
+   pci_read_config_dword(dev, ent_offset, _upper);
+   ent_offset += 4;
+
+   flags |= IORESOURCE_MEM_64;
+
+   /* entry starts above 32-bit boundary, can't use */
+   if (!support_64 && base_upper)
+   goto out;
+
+   if (support_64)
+   start |= ((u64)base_upper << 32);
+   }
+
+

[PATCH 1/2] PCI: Add Enhanced Allocation register entries

[Sean O. Stalley]

Add registers defined in PCI-SIG's Enhanced allocation ECN.

Signed-off-by: Sean O. Stalley 
---
 include/uapi/linux/pci_regs.h | 40 +++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 413417f..084ce98 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -216,7 +216,8 @@
 #define  PCI_CAP_ID_MSIX   0x11/* MSI-X */
 #define  PCI_CAP_ID_SATA   0x12/* SATA Data/Index Conf. */
 #define  PCI_CAP_ID_AF 0x13/* PCI Advanced Features */
-#define  PCI_CAP_ID_MAXPCI_CAP_ID_AF
+#define  PCI_CAP_ID_EA 0x14/* PCI Enhanced Allocation */
+#define  PCI_CAP_ID_MAXPCI_CAP_ID_EA
 #define PCI_CAP_LIST_NEXT  1   /* Next capability in the list */
 #define PCI_CAP_FLAGS  2   /* Capability defined flags (16 bits) */
 #define PCI_CAP_SIZEOF 4
@@ -353,6 +354,43 @@
 #define  PCI_AF_STATUS_TP  0x01
 #define PCI_CAP_AF_SIZEOF  6   /* size of AF registers */
 
+/* PCI Enhanced Allocation registers */
+
+#define PCI_EA_NUM_ENT 2   /* Number of Capability Entries */
+#define PCI_EA_NUM_ENT_MASK0x3f/* Num Entries Mask */
+#define PCI_EA_FIRST_ENT   4   /* First EA Entry in List */
+#define PCI_EA_FIRST_ENT_BRIDGE8   /* First EA Entry for Bridges */
+#define PCI_EA_ES  0x7 /* Entry Size */
+#define PCI_EA_BEI(x)  (((x) >> 4) & 0xf) /* BAR Equivalent Indicator */
+/* 0-5 map to BARs 0-5 respectively */
+#define  PCI_EA_BEI_BRIDGE 6   /* Resource behind bridge */
+#define  PCI_EA_BEI_ENI7   /* Equivalent Not Indicated */
+#define  PCI_EA_BEI_ROM8   /* Expansion ROM */
+/* 9-14 map to VF BARs 0-5 respectively */
+#define  PCI_EA_BEI_RESERVED   15  /* Reserved - Treat like ENI */
+
+#define PCI_EA_PP(x)   (((x) >>  8) & 0xff)/* Primary Properties */
+#define PCI_EA_SP(x)   (((x) >> 16) & 0xff)/* Secondary Properties */
+#define  PCI_EA_P_MEM  0x00/* Non-Prefetch Memory */
+#define  PCI_EA_P_MEM_PREFETCH 0x01/* Prefetchable Memory */
+#define  PCI_EA_P_IO   0x02/* I/O Space */
+#define  PCI_EA_P_VIRT_MEM_PREFETCH0x03/* VF Prefetchable Memory */
+#define  PCI_EA_P_VIRT_MEM 0x04/* VF Non-Prefetch Memory */
+#define  PCI_EA_P_BRIDGE_MEM   0x05/* Bridge Non-Prefetch Memory */
+#define  PCI_EA_P_BRIDGE_MEM_PREFETCH  0x06/* Bridge Prefetchable Memory */
+#define  PCI_EA_P_BRIDGE_IO0x07/* Bridge I/O Space */
+/* 0x08-0xfc reserved */
+#define  PCI_EA_P_MEM_RESERVED 0xfd/* Reserved Memory */
+#define  PCI_EA_P_IO_RESERVED  0xfe/* Reserved I/O Space */
+#define  PCI_EA_P_UNAVAILABLE  0xff/* Entry Unavailable */
+#define PCI_EA_WRITEABLE   BIT(30) /* Writable, 1 = RW, 0 = HwInit */
+#define PCI_EA_ENABLE  BIT(31) /* Enable for this entry */
+#define PCI_EA_BASE4   /* Base Address Offset */
+#define PCI_EA_MAX_OFFSET  8   /* MaxOffset (resource length) */
+/* bit 0 is reserved */
+#define PCI_EA_IS_64   BIT(1)  /* 64-bit field flag */
+#define PCI_EA_FIELD_MASK  0xfffc  /* For Base & Max Offset */
+
 /* PCI-X registers (Type 0 (non-bridge) devices) */
 
 #define PCI_X_CMD  2   /* Modes & Features */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] PCI: Add support for Enhanced Allocation devices

[Sean O. Stalley]

Add support for devices using Enhanced Allocation entries instead of BARs.
This patch allows the kernel to parse the EA Extended Capability structure
in PCI configspace and claim the BAR-equivalent resources.

Signed-off-by: Sean O. Stalley sean.stal...@intel.com
---
 drivers/pci/pci.c   | 219 
 drivers/pci/pci.h   |   1 +
 drivers/pci/probe.c |   3 +
 3 files changed, 223 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0008c95..c8217a8 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2134,6 +2134,225 @@ void pci_pm_init(struct pci_dev *dev)
}
 }
 
+static unsigned long pci_ea_set_flags(struct pci_dev *dev, u8 prop)
+{
+   unsigned long flags = IORESOURCE_PCI_FIXED;
+
+   switch (prop) {
+   case PCI_EA_P_MEM:
+   flags |= IORESOURCE_MEM;
+   break;
+   case PCI_EA_P_MEM_PREFETCH:
+   flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
+   break;
+   case PCI_EA_P_IO:
+   flags |= IORESOURCE_IO;
+   break;
+   default:
+   dev_warn(dev-dev, %s: Property type %x not supported\n,
+   __func__, prop);
+   return 0;
+   }
+
+   return flags;
+}
+
+static struct resource *pci_ea_get_parent_resource(struct pci_dev *dev,
+   struct resource *res)
+{
+   struct resource *parent;
+
+   parent = pci_find_parent_resource(dev, res);
+   if (parent)
+   return parent;
+
+   /* for resources not claimed by a bridge */
+   if (res-flags  IORESOURCE_MEM)
+   return iomem_resource;
+
+   if (res-flags  IORESOURCE_IO)
+   return ioport_resource;
+
+   return NULL;
+}
+
+/* claim the memory for this device in the proper location */
+static void pci_ea_claim_resource(struct pci_dev *dev, struct resource *res)
+{
+   struct resource *parent;
+   struct resource *conflict;
+
+   parent = pci_ea_get_parent_resource(dev, res);
+   if (!parent) {
+   dev_warn(dev-dev, can't find parent resource for EA entry %s 
%pR\n,
+   res-name, res);
+   return;
+   }
+
+   /* claim the appropriate resource */
+   conflict = request_resource_conflict(parent, res);
+   if (conflict) {
+   dev_warn(dev-dev, can't claim EA entry %s %pR: address 
conflict with %s %pR\n,
+res-name, res, conflict-name, conflict);
+   }
+}
+
+static struct resource *pci_ea_get_resource(struct pci_dev *dev, u8 bei)
+{
+   if (bei = PCI_STD_RESOURCE_END)
+   return dev-resource[bei];
+   else if (bei == PCI_EA_BEI_ROM)
+   return dev-resource[PCI_ROM_RESOURCE];
+   else
+   return NULL;
+}
+
+/* Read an Enhanced Allocation (EA) entry */
+static int pci_ea_read(struct pci_dev *dev, int offset)
+{
+   struct resource *res;
+   int ent_offset = offset;
+   int ent_size;
+   resource_size_t start;
+   resource_size_t end;
+   unsigned long flags;
+   u32 dw0;
+   u32 base;
+   u32 max_offset;
+   bool support_64 = (sizeof(resource_size_t) = 8);
+
+   pci_read_config_dword(dev, ent_offset, dw0);
+   ent_offset += 4;
+
+   /* Entry size field indicates DWORDs after 1st */
+   ent_size = ((dw0  PCI_EA_ES) + 1)  2;
+
+   if (!(dw0  PCI_EA_ENABLE)) {
+   dev_err(dev-dev, %s: Entry not enabled\n, __func__);
+   goto out;
+   }
+
+   res = pci_ea_get_resource(dev, PCI_EA_BEI(dw0));
+   if (!res) {
+   dev_err(dev-dev, %s: Unsupported EA entry BEI\n, __func__);
+   goto out;
+   }
+
+   flags = pci_ea_set_flags(dev, PCI_EA_PP(dw0));
+   if (!flags)
+   flags = pci_ea_set_flags(dev, PCI_EA_SP(dw0));
+   if (!flags) {
+   dev_err(dev-dev, %s: Entry EA properties not supported\n,
+   __func__);
+   goto out;
+   }
+
+   /* Read Base */
+   pci_read_config_dword(dev, ent_offset, base);
+   start = (base  PCI_EA_FIELD_MASK);
+   ent_offset += 4;
+
+   /* Read MaxOffset */
+   pci_read_config_dword(dev, ent_offset, max_offset);
+   ent_offset += 4;
+
+   /* Read Base MSBs (if 64-bit entry) */
+   if (base  PCI_EA_IS_64) {
+   u32 base_upper;
+
+   pci_read_config_dword(dev, ent_offset, base_upper);
+   ent_offset += 4;
+
+   flags |= IORESOURCE_MEM_64;
+
+   /* entry starts above 32-bit boundary, can't use */
+   if (!support_64  base_upper)
+   goto out;
+
+   if (support_64)
+   start |= ((u64)base_upper  32);
+   }
+
+   dev_dbg(dev-dev, %s: start = %pa\n, __func__, start);
+
+   end = start + (max_offset | 0x03

[PATCH 1/2] PCI: Add Enhanced Allocation register entries

[Sean O. Stalley]

Add registers defined in PCI-SIG's Enhanced allocation ECN.

Signed-off-by: Sean O. Stalley sean.stal...@intel.com
---
 include/uapi/linux/pci_regs.h | 40 +++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 413417f..084ce98 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -216,7 +216,8 @@
 #define  PCI_CAP_ID_MSIX   0x11/* MSI-X */
 #define  PCI_CAP_ID_SATA   0x12/* SATA Data/Index Conf. */
 #define  PCI_CAP_ID_AF 0x13/* PCI Advanced Features */
-#define  PCI_CAP_ID_MAXPCI_CAP_ID_AF
+#define  PCI_CAP_ID_EA 0x14/* PCI Enhanced Allocation */
+#define  PCI_CAP_ID_MAXPCI_CAP_ID_EA
 #define PCI_CAP_LIST_NEXT  1   /* Next capability in the list */
 #define PCI_CAP_FLAGS  2   /* Capability defined flags (16 bits) */
 #define PCI_CAP_SIZEOF 4
@@ -353,6 +354,43 @@
 #define  PCI_AF_STATUS_TP  0x01
 #define PCI_CAP_AF_SIZEOF  6   /* size of AF registers */
 
+/* PCI Enhanced Allocation registers */
+
+#define PCI_EA_NUM_ENT 2   /* Number of Capability Entries */
+#define PCI_EA_NUM_ENT_MASK0x3f/* Num Entries Mask */
+#define PCI_EA_FIRST_ENT   4   /* First EA Entry in List */
+#define PCI_EA_FIRST_ENT_BRIDGE8   /* First EA Entry for Bridges */
+#define PCI_EA_ES  0x7 /* Entry Size */
+#define PCI_EA_BEI(x)  (((x)  4)  0xf) /* BAR Equivalent Indicator */
+/* 0-5 map to BARs 0-5 respectively */
+#define  PCI_EA_BEI_BRIDGE 6   /* Resource behind bridge */
+#define  PCI_EA_BEI_ENI7   /* Equivalent Not Indicated */
+#define  PCI_EA_BEI_ROM8   /* Expansion ROM */
+/* 9-14 map to VF BARs 0-5 respectively */
+#define  PCI_EA_BEI_RESERVED   15  /* Reserved - Treat like ENI */
+
+#define PCI_EA_PP(x)   (((x)   8)  0xff)/* Primary Properties */
+#define PCI_EA_SP(x)   (((x)  16)  0xff)/* Secondary Properties */
+#define  PCI_EA_P_MEM  0x00/* Non-Prefetch Memory */
+#define  PCI_EA_P_MEM_PREFETCH 0x01/* Prefetchable Memory */
+#define  PCI_EA_P_IO   0x02/* I/O Space */
+#define  PCI_EA_P_VIRT_MEM_PREFETCH0x03/* VF Prefetchable Memory */
+#define  PCI_EA_P_VIRT_MEM 0x04/* VF Non-Prefetch Memory */
+#define  PCI_EA_P_BRIDGE_MEM   0x05/* Bridge Non-Prefetch Memory */
+#define  PCI_EA_P_BRIDGE_MEM_PREFETCH  0x06/* Bridge Prefetchable Memory */
+#define  PCI_EA_P_BRIDGE_IO0x07/* Bridge I/O Space */
+/* 0x08-0xfc reserved */
+#define  PCI_EA_P_MEM_RESERVED 0xfd/* Reserved Memory */
+#define  PCI_EA_P_IO_RESERVED  0xfe/* Reserved I/O Space */
+#define  PCI_EA_P_UNAVAILABLE  0xff/* Entry Unavailable */
+#define PCI_EA_WRITEABLE   BIT(30) /* Writable, 1 = RW, 0 = HwInit */
+#define PCI_EA_ENABLE  BIT(31) /* Enable for this entry */
+#define PCI_EA_BASE4   /* Base Address Offset */
+#define PCI_EA_MAX_OFFSET  8   /* MaxOffset (resource length) */
+/* bit 0 is reserved */
+#define PCI_EA_IS_64   BIT(1)  /* 64-bit field flag */
+#define PCI_EA_FIELD_MASK  0xfffc  /* For Base  Max Offset */
+
 /* PCI-X registers (Type 0 (non-bridge) devices) */
 
 #define PCI_X_CMD  2   /* Modes  Features */
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/2] PCI: Add support for PCI Enhanced Allocation BARs

[Sean O. Stalley]

PCI Enhanced Allocation is a new method of allocating MMIO  IO
resources for PCI devices  bridges. It can be used instead
of the traditional PCI method of using BARs.

EA entries are hardware-initialized to a fixed address.
Unlike BARs, regions described by EA are cannot be moved.
Because of this, only devices which are permanently connected to
the PCI bus can use EA. A removable PCI card must not use EA.

This patchset adds support for using EA entries instead of BARs.

The Enhanced Allocation ECN is publicly available here:
https://www.pcisig.com/specifications/conventional/ECN_Enhanced_Allocation_23_Oct_2014_Final.pdf

Sean O. Stalley (2):
  PCI: Add Enhanced Allocation register entries
  PCI: Add parsing of Enhanced Allocation entries

 drivers/pci/pci.c | 219 ++
 drivers/pci/pci.h |   1 +
 drivers/pci/probe.c   |   3 +
 include/uapi/linux/pci_regs.h |  40 +++-
 4 files changed, 262 insertions(+), 1 deletion(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 3/4] pci: mm: Add pci_pool_zalloc() call

[Sean O. Stalley]

Add a wrapper function for pci_pool_alloc() to get zeroed memory.

Signed-off-by: Sean O. Stalley 
---
 include/linux/pci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 755a2cd..e6ec7d9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1176,6 +1176,8 @@ int pci_set_vga_state(struct pci_dev *pdev, bool decode,
dma_pool_create(name, >dev, size, align, allocation)
 #definepci_pool_destroy(pool) dma_pool_destroy(pool)
 #definepci_pool_alloc(pool, flags, handle) dma_pool_alloc(pool, flags, 
handle)
+#definepci_pool_zalloc(pool, flags, handle) \
+   dma_pool_zalloc(pool, flags, handle)
 #definepci_pool_free(pool, vaddr, addr) dma_pool_free(pool, vaddr, 
addr)
 
 enum pci_dma_burst_strategy {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 4/4] coccinelle: mm: scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

[Sean O. Stalley]

add [pci|dma]_pool_zalloc coccinelle check.
replaces instances of [pci|dma]_pool_alloc() followed by memset(0)
with [pci|dma]_pool_zalloc().

Signed-off-by: Sean O. Stalley 
---
 .../coccinelle/api/alloc/pool_zalloc-simple.cocci  | 84 ++
 1 file changed, 84 insertions(+)
 create mode 100644 scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

diff --git a/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci 
b/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci
new file mode 100644
index 000..9b7eb32
--- /dev/null
+++ b/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci
@@ -0,0 +1,84 @@
+///
+/// Use *_pool_zalloc rather than *_pool_alloc followed by memset with 0
+///
+// Copyright: (C) 2015 Intel Corp.  GPLv2.
+// Options: --no-includes --include-headers
+//
+// Keywords: dma_pool_zalloc, pci_pool_zalloc
+//
+
+virtual context
+virtual patch
+virtual org
+virtual report
+
+//--
+//  For context mode
+//--
+
+@depends on context@
+expression x;
+statement S;
+@@
+
+* x = \(dma_pool_alloc\|pci_pool_alloc\)(...);
+  if ((x==NULL) || ...) S
+* memset(x,0, ...);
+
+//--
+//  For patch mode
+//--
+
+@depends on patch@
+expression x;
+expression a,b,c;
+statement S;
+@@
+
+- x = dma_pool_alloc(a,b,c);
++ x = dma_pool_zalloc(a,b,c);
+  if ((x==NULL) || ...) S
+- memset(x,0,...);
+
+@depends on patch@
+expression x;
+expression a,b,c;
+statement S;
+@@
+
+- x = pci_pool_alloc(a,b,c);
++ x = pci_pool_zalloc(a,b,c);
+  if ((x==NULL) || ...) S
+- memset(x,0,...);
+
+//--
+//  For org and report mode
+//--
+
+@r depends on org || report@
+expression x;
+expression a,b,c;
+statement S;
+position p;
+@@
+
+ x = @p\(dma_pool_alloc\|pci_pool_alloc\)(a,b,c);
+ if ((x==NULL) || ...) S
+ memset(x,0, ...);
+
+@script:python depends on org@
+p << r.p;
+x << r.x;
+@@
+
+msg="%s" % (x)
+msg_safe=msg.replace("[","@(").replace("]",")")
+coccilib.org.print_todo(p[0], msg_safe)
+
+@script:python depends on report@
+p << r.p;
+x << r.x;
+@@
+
+msg="WARNING: *_pool_zalloc should be used for %s, instead of 
*_pool_alloc/memset" % (x)
+coccilib.report.print_report(p[0], msg)
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/4] mm: Add support for __GFP_ZERO flag to dma_pool_alloc()

[Sean O. Stalley]

Currently the __GFP_ZERO flag is ignored by dma_pool_alloc().
Make dma_pool_alloc() zero the memory if this flag is set.

Signed-off-by: Sean O. Stalley 
---
 mm/dmapool.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index fd5fe43..bd49386 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -334,7 +334,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
/* pool_alloc_page() might sleep, so temporarily drop >lock */
spin_unlock_irqrestore(>lock, flags);
 
-   page = pool_alloc_page(pool, mem_flags);
+   page = pool_alloc_page(pool, mem_flags & (~__GFP_ZERO));
if (!page)
return NULL;
 
@@ -372,9 +372,14 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t 
mem_flags,
break;
}
}
-   memset(retval, POOL_POISON_ALLOCATED, pool->size);
+   if (!(mem_flags & __GFP_ZERO))
+   memset(retval, POOL_POISON_ALLOCATED, pool->size);
 #endif
spin_unlock_irqrestore(>lock, flags);
+
+   if (mem_flags & __GFP_ZERO)
+   memset(retval, 0, pool->size);
+
return retval;
 }
 EXPORT_SYMBOL(dma_pool_alloc);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 2/4] mm: Add dma_pool_zalloc() call to DMA API

[Sean O. Stalley]

Add a wrapper function for dma_pool_alloc() to get zeroed memory.

Signed-off-by: Sean O. Stalley 
---
 Documentation/DMA-API.txt | 7 +++
 include/linux/dmapool.h   | 6 ++
 2 files changed, 13 insertions(+)

diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index 5208840..988f757 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -104,6 +104,13 @@ crossing restrictions, pass 0 for alloc; passing 4096 says 
memory allocated
 from this pool must not cross 4KByte boundaries.
 
 
+   void *dma_pool_zalloc(struct dma_pool *pool, gfp_t mem_flags,
+ dma_addr_t *handle)
+
+Wraps dma_pool_alloc() and also zeroes the returned memory if the
+allocation attempt succeeded.
+
+
void *dma_pool_alloc(struct dma_pool *pool, gfp_t gfp_flags,
dma_addr_t *dma_handle);
 
diff --git a/include/linux/dmapool.h b/include/linux/dmapool.h
index 022e34f..6d8079b 100644
--- a/include/linux/dmapool.h
+++ b/include/linux/dmapool.h
@@ -22,6 +22,12 @@ void dma_pool_destroy(struct dma_pool *pool);
 void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 dma_addr_t *handle);
 
+static inline void *dma_pool_zalloc(struct dma_pool *pool, gfp_t mem_flags,
+   dma_addr_t *handle)
+{
+   return dma_pool_alloc(pool, mem_flags | __GFP_ZERO, handle);
+}
+
 void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t addr);
 
 /*
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 0/4] mm: add dma_pool_zalloc() & pci_pool_zalloc()

[Sean O. Stalley]

Currently a call to dma_pool_alloc() with a ___GFP_ZERO flag returns
a non-zeroed memory region.

This patchset adds support for the __GFP_ZERO flag to dma_pool_alloc(),
adds 2 wrapper functions for allocing zeroed memory from a pool, 
and provides a coccinelle script for finding & replacing instances of
dma_pool_alloc() followed by memset(0) with a single dma_pool_zalloc() call.

Changes from v1 to v2:
- don't memset() POOL_POISON_ALLOCATED in dma_pool_alloc() if
  __GFP_ZERO is set
- Ran test to see how often pool_alloc_page() is called


There was some concern that this always calls memset() to zero,
instead of passing __GFP_ZERO into the page allocator.
[https://lkml.org/lkml/2015/7/15/881]

I ran a test on my system to get an idea of how often dma_pool_alloc()
calls into pool_alloc_page().

After Boot: [   30.119863] alloc_calls:541, page_allocs:7
After an hour:  [ 3600.951031] alloc_calls:9566, page_allocs:12
After copying 1GB file onto a USB drive:
[ 4260.657148] alloc_calls:17225, page_allocs:12

It doesn't look like dma_pool_alloc() calls down to the page allocator
very often (at least on my system).


Sean O. Stalley (4):
  mm: Add support for __GFP_ZERO flag to dma_pool_alloc()
  mm: Add dma_pool_zalloc() call to DMA API
  pci: mm: Add pci_pool_zalloc() call
  coccinelle: mm: scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

 Documentation/DMA-API.txt  |  7 ++
 include/linux/dmapool.h|  6 ++
 include/linux/pci.h|  2 +
 mm/dmapool.c   |  9 ++-
 .../coccinelle/api/alloc/pool_zalloc-simple.cocci  | 84 ++
 5 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100644 scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 3/4] pci: mm: Add pci_pool_zalloc() call

[Sean O. Stalley]

Add a wrapper function for pci_pool_alloc() to get zeroed memory.

Signed-off-by: Sean O. Stalley sean.stal...@intel.com
---
 include/linux/pci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 755a2cd..e6ec7d9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1176,6 +1176,8 @@ int pci_set_vga_state(struct pci_dev *pdev, bool decode,
dma_pool_create(name, pdev-dev, size, align, allocation)
 #definepci_pool_destroy(pool) dma_pool_destroy(pool)
 #definepci_pool_alloc(pool, flags, handle) dma_pool_alloc(pool, flags, 
handle)
+#definepci_pool_zalloc(pool, flags, handle) \
+   dma_pool_zalloc(pool, flags, handle)
 #definepci_pool_free(pool, vaddr, addr) dma_pool_free(pool, vaddr, 
addr)
 
 enum pci_dma_burst_strategy {
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 4/4] coccinelle: mm: scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

[Sean O. Stalley]

add [pci|dma]_pool_zalloc coccinelle check.
replaces instances of [pci|dma]_pool_alloc() followed by memset(0)
with [pci|dma]_pool_zalloc().

Signed-off-by: Sean O. Stalley sean.stal...@intel.com
---
 .../coccinelle/api/alloc/pool_zalloc-simple.cocci  | 84 ++
 1 file changed, 84 insertions(+)
 create mode 100644 scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

diff --git a/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci 
b/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci
new file mode 100644
index 000..9b7eb32
--- /dev/null
+++ b/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci
@@ -0,0 +1,84 @@
+///
+/// Use *_pool_zalloc rather than *_pool_alloc followed by memset with 0
+///
+// Copyright: (C) 2015 Intel Corp.  GPLv2.
+// Options: --no-includes --include-headers
+//
+// Keywords: dma_pool_zalloc, pci_pool_zalloc
+//
+
+virtual context
+virtual patch
+virtual org
+virtual report
+
+//--
+//  For context mode
+//--
+
+@depends on context@
+expression x;
+statement S;
+@@
+
+* x = \(dma_pool_alloc\|pci_pool_alloc\)(...);
+  if ((x==NULL) || ...) S
+* memset(x,0, ...);
+
+//--
+//  For patch mode
+//--
+
+@depends on patch@
+expression x;
+expression a,b,c;
+statement S;
+@@
+
+- x = dma_pool_alloc(a,b,c);
++ x = dma_pool_zalloc(a,b,c);
+  if ((x==NULL) || ...) S
+- memset(x,0,...);
+
+@depends on patch@
+expression x;
+expression a,b,c;
+statement S;
+@@
+
+- x = pci_pool_alloc(a,b,c);
++ x = pci_pool_zalloc(a,b,c);
+  if ((x==NULL) || ...) S
+- memset(x,0,...);
+
+//--
+//  For org and report mode
+//--
+
+@r depends on org || report@
+expression x;
+expression a,b,c;
+statement S;
+position p;
+@@
+
+ x = @p\(dma_pool_alloc\|pci_pool_alloc\)(a,b,c);
+ if ((x==NULL) || ...) S
+ memset(x,0, ...);
+
+@script:python depends on org@
+p  r.p;
+x  r.x;
+@@
+
+msg=%s % (x)
+msg_safe=msg.replace([,@().replace(],))
+coccilib.org.print_todo(p[0], msg_safe)
+
+@script:python depends on report@
+p  r.p;
+x  r.x;
+@@
+
+msg=WARNING: *_pool_zalloc should be used for %s, instead of 
*_pool_alloc/memset % (x)
+coccilib.report.print_report(p[0], msg)
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/4] mm: Add support for __GFP_ZERO flag to dma_pool_alloc()

[Sean O. Stalley]

Currently the __GFP_ZERO flag is ignored by dma_pool_alloc().
Make dma_pool_alloc() zero the memory if this flag is set.

Signed-off-by: Sean O. Stalley sean.stal...@intel.com
---
 mm/dmapool.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index fd5fe43..bd49386 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -334,7 +334,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
/* pool_alloc_page() might sleep, so temporarily drop pool-lock */
spin_unlock_irqrestore(pool-lock, flags);
 
-   page = pool_alloc_page(pool, mem_flags);
+   page = pool_alloc_page(pool, mem_flags  (~__GFP_ZERO));
if (!page)
return NULL;
 
@@ -372,9 +372,14 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t 
mem_flags,
break;
}
}
-   memset(retval, POOL_POISON_ALLOCATED, pool-size);
+   if (!(mem_flags  __GFP_ZERO))
+   memset(retval, POOL_POISON_ALLOCATED, pool-size);
 #endif
spin_unlock_irqrestore(pool-lock, flags);
+
+   if (mem_flags  __GFP_ZERO)
+   memset(retval, 0, pool-size);
+
return retval;
 }
 EXPORT_SYMBOL(dma_pool_alloc);
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 2/4] mm: Add dma_pool_zalloc() call to DMA API

[Sean O. Stalley]

Add a wrapper function for dma_pool_alloc() to get zeroed memory.

Signed-off-by: Sean O. Stalley sean.stal...@intel.com
---
 Documentation/DMA-API.txt | 7 +++
 include/linux/dmapool.h   | 6 ++
 2 files changed, 13 insertions(+)

diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index 5208840..988f757 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -104,6 +104,13 @@ crossing restrictions, pass 0 for alloc; passing 4096 says 
memory allocated
 from this pool must not cross 4KByte boundaries.
 
 
+   void *dma_pool_zalloc(struct dma_pool *pool, gfp_t mem_flags,
+ dma_addr_t *handle)
+
+Wraps dma_pool_alloc() and also zeroes the returned memory if the
+allocation attempt succeeded.
+
+
void *dma_pool_alloc(struct dma_pool *pool, gfp_t gfp_flags,
dma_addr_t *dma_handle);
 
diff --git a/include/linux/dmapool.h b/include/linux/dmapool.h
index 022e34f..6d8079b 100644
--- a/include/linux/dmapool.h
+++ b/include/linux/dmapool.h
@@ -22,6 +22,12 @@ void dma_pool_destroy(struct dma_pool *pool);
 void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 dma_addr_t *handle);
 
+static inline void *dma_pool_zalloc(struct dma_pool *pool, gfp_t mem_flags,
+   dma_addr_t *handle)
+{
+   return dma_pool_alloc(pool, mem_flags | __GFP_ZERO, handle);
+}
+
 void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t addr);
 
 /*
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 0/4] mm: add dma_pool_zalloc() pci_pool_zalloc()

[Sean O. Stalley]

Currently a call to dma_pool_alloc() with a ___GFP_ZERO flag returns
a non-zeroed memory region.

This patchset adds support for the __GFP_ZERO flag to dma_pool_alloc(),
adds 2 wrapper functions for allocing zeroed memory from a pool, 
and provides a coccinelle script for finding  replacing instances of
dma_pool_alloc() followed by memset(0) with a single dma_pool_zalloc() call.

Changes from v1 to v2:
- don't memset() POOL_POISON_ALLOCATED in dma_pool_alloc() if
  __GFP_ZERO is set
- Ran test to see how often pool_alloc_page() is called


There was some concern that this always calls memset() to zero,
instead of passing __GFP_ZERO into the page allocator.
[https://lkml.org/lkml/2015/7/15/881]

I ran a test on my system to get an idea of how often dma_pool_alloc()
calls into pool_alloc_page().

After Boot: [   30.119863] alloc_calls:541, page_allocs:7
After an hour:  [ 3600.951031] alloc_calls:9566, page_allocs:12
After copying 1GB file onto a USB drive:
[ 4260.657148] alloc_calls:17225, page_allocs:12

It doesn't look like dma_pool_alloc() calls down to the page allocator
very often (at least on my system).


Sean O. Stalley (4):
  mm: Add support for __GFP_ZERO flag to dma_pool_alloc()
  mm: Add dma_pool_zalloc() call to DMA API
  pci: mm: Add pci_pool_zalloc() call
  coccinelle: mm: scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

 Documentation/DMA-API.txt  |  7 ++
 include/linux/dmapool.h|  6 ++
 include/linux/pci.h|  2 +
 mm/dmapool.c   |  9 ++-
 .../coccinelle/api/alloc/pool_zalloc-simple.cocci  | 84 ++
 5 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100644 scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Sean O. Stalley]

On Wed, Jul 22, 2015 at 11:55:32AM -0500, Jeremy White wrote:
> I privately wrote to the Intel authors of that patch a week ago; I've
> publicly included them in this thread as well.  As far as I can tell,
> they've been silent on this front since November; I fear that they may
> have moved on, or that Intel is not actively working on this.  None of
> the Intel authors listed on the MA-USB specification are kernel
> contributors, so I did not have a way to reach out to them.  If you have
> the means to engage others, I would appreciate that.

Sorry for the delay. The short answer is: Yes, we have been actively working on 
this driver.
Per Greg KH's request, we have been cleaning up the driver internally.
There was a lot to clean up, which is why we have been silent on LKML.

-Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/1] Add a usbredir kernel module to remotely connect USB devices over IP.

[Sean O. Stalley]

On Wed, Jul 22, 2015 at 11:55:32AM -0500, Jeremy White wrote:
 I privately wrote to the Intel authors of that patch a week ago; I've
 publicly included them in this thread as well.  As far as I can tell,
 they've been silent on this front since November; I fear that they may
 have moved on, or that Intel is not actively working on this.  None of
 the Intel authors listed on the MA-USB specification are kernel
 contributors, so I did not have a way to reach out to them.  If you have
 the means to engage others, I would appreciate that.

Sorry for the delay. The short answer is: Yes, we have been actively working on 
this driver.
Per Greg KH's request, we have been cleaning up the driver internally.
There was a lot to clean up, which is why we have been silent on LKML.

-Sean
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mm: Add support for __GFP_ZERO flag to dma_pool_alloc()

[Sean O. Stalley]

Thanks for the review Andrew, my responses are inline.

-Sean

On Wed, Jul 15, 2015 at 02:29:07PM -0700, Andrew Morton wrote:
> On Wed, 15 Jul 2015 14:14:40 -0700 "Sean O. Stalley"  
> wrote:
> 
> > Currently the __GFP_ZERO flag is ignored by dma_pool_alloc().
> > Make dma_pool_alloc() zero the memory if this flag is set.
> > 
> > ...
> >
> > --- a/mm/dmapool.c
> > +++ b/mm/dmapool.c
> > @@ -334,7 +334,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t 
> > mem_flags,
> > /* pool_alloc_page() might sleep, so temporarily drop >lock */
> > spin_unlock_irqrestore(>lock, flags);
> >  
> > -   page = pool_alloc_page(pool, mem_flags);
> > +   page = pool_alloc_page(pool, mem_flags & (~__GFP_ZERO));
> > if (!page)
> > return NULL;
> >  
> > @@ -375,6 +375,10 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t 
> > mem_flags,
> > memset(retval, POOL_POISON_ALLOCATED, pool->size);
> >  #endif
> > spin_unlock_irqrestore(>lock, flags);
> > +
> > +   if (mem_flags & __GFP_ZERO)
> > +   memset(retval, 0, pool->size);
> > +
> > return retval;
> >  }
> >  EXPORT_SYMBOL(dma_pool_alloc);
> 
> hm, this code is all a bit confused.
> 
> We'd really prefer that the __GFP_ZERO be passed all the way to the
> bottom level, so that places which are responsible for zeroing memory
> (eg, the page allocator) can do their designated function.  One reason
> for this is that if someone comes up with a whizzy way of zeroing
> memory on their architecture (eg, non-temporal store) then that will be
> implemented in the core page allocator and the dma code will miss out.

It would be nice if we could use the page allocator for whizzy zeroing.
There are a few reasons why I didn't pass __GFP_ZERO down to the allocator:

 - dma_pool_alloc() reuses blocks of memory that were recently freed by 
dma_pool_free().
   We have to memset(0) old blocks, since we don't know what's in them.

 - When a new page is alloced, pool_initalize_page() writes an integer to every 
block.
   So even if we passed __GFP_ZERO down to the allocator, the block would not 
be empty
   by the time dma_pool_alloc() returns.

 - Assuming a driver is allocing as often as it is freeing,
   once the pool has enough memory it shouldn't call down to the allocator very 
often,
   so any optimization down in the allocator shouldn't make much of a difference

> Also, and just from a brief look around,
> drivers/base/dma-coherent.c:dma_alloc_from_coherent() is already
> zeroing the memory so under some circumstances I think we'll zero the
> memory twice?  We could fix that by passing the gfp_t to
> dma_alloc_from_coherent() and then changing dma_alloc_from_coherent()
> to *not* zero the memory if __GFP_ZERO, but wouldn't that be peculiar?

I noticed this as well. In this case, we would be zeroing twice.
This is no worse than the current case (where dma_pool_alloc() returns,
then the driver calls memset(0)).

> Also, passing __GFP_ZERO will now cause pool_alloc_page()'s
> memset(POOL_POISON_FREED) to be wiped out.  I guess that's harmless,
> but a bit inefficient?

Inefficient, but no more inefficient than the current case.
I didn't think it would be a problem (since it only happens if dma pool 
debuging is enabled).
I could add a check to only memset the poison if __GFP_ZERO is not set.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/4] mm: Add dma_pool_zalloc() call to DMA API

[Sean O. Stalley]

Add a wrapper function for dma_pool_alloc() to get zeroed memory.

Signed-off-by: Sean O. Stalley 
---
 Documentation/DMA-API.txt | 7 +++
 include/linux/dmapool.h   | 6 ++
 2 files changed, 13 insertions(+)

diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index 5208840..988f757 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -104,6 +104,13 @@ crossing restrictions, pass 0 for alloc; passing 4096 says 
memory allocated
 from this pool must not cross 4KByte boundaries.
 
 
+   void *dma_pool_zalloc(struct dma_pool *pool, gfp_t mem_flags,
+ dma_addr_t *handle)
+
+Wraps dma_pool_alloc() and also zeroes the returned memory if the
+allocation attempt succeeded.
+
+
void *dma_pool_alloc(struct dma_pool *pool, gfp_t gfp_flags,
dma_addr_t *dma_handle);
 
diff --git a/include/linux/dmapool.h b/include/linux/dmapool.h
index 022e34f..6d8079b 100644
--- a/include/linux/dmapool.h
+++ b/include/linux/dmapool.h
@@ -22,6 +22,12 @@ void dma_pool_destroy(struct dma_pool *pool);
 void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 dma_addr_t *handle);
 
+static inline void *dma_pool_zalloc(struct dma_pool *pool, gfp_t mem_flags,
+   dma_addr_t *handle)
+{
+   return dma_pool_alloc(pool, mem_flags | __GFP_ZERO, handle);
+}
+
 void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t addr);
 
 /*
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/4] pci: mm: Add pci_pool_zalloc() call

[Sean O. Stalley]

Add a wrapper function for pci_pool_alloc() to get zeroed memory.

Signed-off-by: Sean O. Stalley 
---
 include/linux/pci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 755a2cd..e6ec7d9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1176,6 +1176,8 @@ int pci_set_vga_state(struct pci_dev *pdev, bool decode,
dma_pool_create(name, >dev, size, align, allocation)
 #definepci_pool_destroy(pool) dma_pool_destroy(pool)
 #definepci_pool_alloc(pool, flags, handle) dma_pool_alloc(pool, flags, 
handle)
+#definepci_pool_zalloc(pool, flags, handle) \
+   dma_pool_zalloc(pool, flags, handle)
 #definepci_pool_free(pool, vaddr, addr) dma_pool_free(pool, vaddr, 
addr)
 
 enum pci_dma_burst_strategy {
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/4] mm: add dma_pool_zalloc() & pci_pool_zalloc()

[Sean O. Stalley]

Currently a call to dma_pool_alloc() with a ___GFP_ZERO flag returns
a non-zeroed memory region.

This patchset adds support for the ___GFP_ZERO flag to dma_pool_alloc(),
adds 2 wrapper functions for allocing zeroed memory from a pool, 
and provides a coccinelle script for finding & replacing instances of
dma_pool_alloc() followed by memset(0) with a single dma_pool_zalloc() call.

Sean O. Stalley (4):
  mm: Add support for __GFP_ZERO flag to dma_pool_alloc()
  mm: Add dma_pool_zalloc() call to DMA API
  pci: mm: Add pci_pool_zalloc() call
  coccinelle: mm: scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

 Documentation/DMA-API.txt  |  7 ++
 include/linux/dmapool.h|  6 ++
 include/linux/pci.h|  2 +
 mm/dmapool.c   |  6 +-
 .../coccinelle/api/alloc/pool_zalloc-simple.cocci  | 84 ++
 5 files changed, 104 insertions(+), 1 deletion(-)
 create mode 100644 scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/4] mm: Add support for __GFP_ZERO flag to dma_pool_alloc()

[Sean O. Stalley]

Currently the __GFP_ZERO flag is ignored by dma_pool_alloc().
Make dma_pool_alloc() zero the memory if this flag is set.

Signed-off-by: Sean O. Stalley 
---
 mm/dmapool.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index fd5fe43..449a5d09 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -334,7 +334,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
/* pool_alloc_page() might sleep, so temporarily drop >lock */
spin_unlock_irqrestore(>lock, flags);
 
-   page = pool_alloc_page(pool, mem_flags);
+   page = pool_alloc_page(pool, mem_flags & (~__GFP_ZERO));
if (!page)
return NULL;
 
@@ -375,6 +375,10 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t 
mem_flags,
memset(retval, POOL_POISON_ALLOCATED, pool->size);
 #endif
spin_unlock_irqrestore(>lock, flags);
+
+   if (mem_flags & __GFP_ZERO)
+   memset(retval, 0, pool->size);
+
return retval;
 }
 EXPORT_SYMBOL(dma_pool_alloc);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 4/4] coccinelle: mm: scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

[Sean O. Stalley]

add [pci|dma]_pool_zalloc coccinelle check.
replaces instances of [pci|dma]_pool_alloc() followed by memset(0)
with [pci|dma]_pool_zalloc().

Signed-off-by: Sean O. Stalley 
---
 .../coccinelle/api/alloc/pool_zalloc-simple.cocci  | 84 ++
 1 file changed, 84 insertions(+)
 create mode 100644 scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

diff --git a/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci 
b/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci
new file mode 100644
index 000..9b7eb32
--- /dev/null
+++ b/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci
@@ -0,0 +1,84 @@
+///
+/// Use *_pool_zalloc rather than *_pool_alloc followed by memset with 0
+///
+// Copyright: (C) 2015 Intel Corp.  GPLv2.
+// Options: --no-includes --include-headers
+//
+// Keywords: dma_pool_zalloc, pci_pool_zalloc
+//
+
+virtual context
+virtual patch
+virtual org
+virtual report
+
+//--
+//  For context mode
+//--
+
+@depends on context@
+expression x;
+statement S;
+@@
+
+* x = \(dma_pool_alloc\|pci_pool_alloc\)(...);
+  if ((x==NULL) || ...) S
+* memset(x,0, ...);
+
+//--
+//  For patch mode
+//--
+
+@depends on patch@
+expression x;
+expression a,b,c;
+statement S;
+@@
+
+- x = dma_pool_alloc(a,b,c);
++ x = dma_pool_zalloc(a,b,c);
+  if ((x==NULL) || ...) S
+- memset(x,0,...);
+
+@depends on patch@
+expression x;
+expression a,b,c;
+statement S;
+@@
+
+- x = pci_pool_alloc(a,b,c);
++ x = pci_pool_zalloc(a,b,c);
+  if ((x==NULL) || ...) S
+- memset(x,0,...);
+
+//--
+//  For org and report mode
+//--
+
+@r depends on org || report@
+expression x;
+expression a,b,c;
+statement S;
+position p;
+@@
+
+ x = @p\(dma_pool_alloc\|pci_pool_alloc\)(a,b,c);
+ if ((x==NULL) || ...) S
+ memset(x,0, ...);
+
+@script:python depends on org@
+p << r.p;
+x << r.x;
+@@
+
+msg="%s" % (x)
+msg_safe=msg.replace("[","@(").replace("]",")")
+coccilib.org.print_todo(p[0], msg_safe)
+
+@script:python depends on report@
+p << r.p;
+x << r.x;
+@@
+
+msg="WARNING: *_pool_zalloc should be used for %s, instead of 
*_pool_alloc/memset" % (x)
+coccilib.report.print_report(p[0], msg)
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] mm: Add support for __GFP_ZERO flag to dma_pool_alloc()

[Sean O. Stalley]

Thanks for the review Andrew, my responses are inline.

-Sean

On Wed, Jul 15, 2015 at 02:29:07PM -0700, Andrew Morton wrote:
 On Wed, 15 Jul 2015 14:14:40 -0700 Sean O. Stalley sean.stal...@intel.com 
 wrote:
 
  Currently the __GFP_ZERO flag is ignored by dma_pool_alloc().
  Make dma_pool_alloc() zero the memory if this flag is set.
  
  ...
 
  --- a/mm/dmapool.c
  +++ b/mm/dmapool.c
  @@ -334,7 +334,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t 
  mem_flags,
  /* pool_alloc_page() might sleep, so temporarily drop pool-lock */
  spin_unlock_irqrestore(pool-lock, flags);
   
  -   page = pool_alloc_page(pool, mem_flags);
  +   page = pool_alloc_page(pool, mem_flags  (~__GFP_ZERO));
  if (!page)
  return NULL;
   
  @@ -375,6 +375,10 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t 
  mem_flags,
  memset(retval, POOL_POISON_ALLOCATED, pool-size);
   #endif
  spin_unlock_irqrestore(pool-lock, flags);
  +
  +   if (mem_flags  __GFP_ZERO)
  +   memset(retval, 0, pool-size);
  +
  return retval;
   }
   EXPORT_SYMBOL(dma_pool_alloc);
 
 hm, this code is all a bit confused.
 
 We'd really prefer that the __GFP_ZERO be passed all the way to the
 bottom level, so that places which are responsible for zeroing memory
 (eg, the page allocator) can do their designated function.  One reason
 for this is that if someone comes up with a whizzy way of zeroing
 memory on their architecture (eg, non-temporal store) then that will be
 implemented in the core page allocator and the dma code will miss out.

It would be nice if we could use the page allocator for whizzy zeroing.
There are a few reasons why I didn't pass __GFP_ZERO down to the allocator:

 - dma_pool_alloc() reuses blocks of memory that were recently freed by 
dma_pool_free().
   We have to memset(0) old blocks, since we don't know what's in them.

 - When a new page is alloced, pool_initalize_page() writes an integer to every 
block.
   So even if we passed __GFP_ZERO down to the allocator, the block would not 
be empty
   by the time dma_pool_alloc() returns.

 - Assuming a driver is allocing as often as it is freeing,
   once the pool has enough memory it shouldn't call down to the allocator very 
often,
   so any optimization down in the allocator shouldn't make much of a difference

 Also, and just from a brief look around,
 drivers/base/dma-coherent.c:dma_alloc_from_coherent() is already
 zeroing the memory so under some circumstances I think we'll zero the
 memory twice?  We could fix that by passing the gfp_t to
 dma_alloc_from_coherent() and then changing dma_alloc_from_coherent()
 to *not* zero the memory if __GFP_ZERO, but wouldn't that be peculiar?

I noticed this as well. In this case, we would be zeroing twice.
This is no worse than the current case (where dma_pool_alloc() returns,
then the driver calls memset(0)).

 Also, passing __GFP_ZERO will now cause pool_alloc_page()'s
 memset(POOL_POISON_FREED) to be wiped out.  I guess that's harmless,
 but a bit inefficient?

Inefficient, but no more inefficient than the current case.
I didn't think it would be a problem (since it only happens if dma pool 
debuging is enabled).
I could add a check to only memset the poison if __GFP_ZERO is not set.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/4] mm: Add support for __GFP_ZERO flag to dma_pool_alloc()

[Sean O. Stalley]

Currently the __GFP_ZERO flag is ignored by dma_pool_alloc().
Make dma_pool_alloc() zero the memory if this flag is set.

Signed-off-by: Sean O. Stalley sean.stal...@intel.com
---
 mm/dmapool.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index fd5fe43..449a5d09 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -334,7 +334,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
/* pool_alloc_page() might sleep, so temporarily drop pool-lock */
spin_unlock_irqrestore(pool-lock, flags);
 
-   page = pool_alloc_page(pool, mem_flags);
+   page = pool_alloc_page(pool, mem_flags  (~__GFP_ZERO));
if (!page)
return NULL;
 
@@ -375,6 +375,10 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t 
mem_flags,
memset(retval, POOL_POISON_ALLOCATED, pool-size);
 #endif
spin_unlock_irqrestore(pool-lock, flags);
+
+   if (mem_flags  __GFP_ZERO)
+   memset(retval, 0, pool-size);
+
return retval;
 }
 EXPORT_SYMBOL(dma_pool_alloc);
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 4/4] coccinelle: mm: scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

[Sean O. Stalley]

add [pci|dma]_pool_zalloc coccinelle check.
replaces instances of [pci|dma]_pool_alloc() followed by memset(0)
with [pci|dma]_pool_zalloc().

Signed-off-by: Sean O. Stalley sean.stal...@intel.com
---
 .../coccinelle/api/alloc/pool_zalloc-simple.cocci  | 84 ++
 1 file changed, 84 insertions(+)
 create mode 100644 scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

diff --git a/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci 
b/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci
new file mode 100644
index 000..9b7eb32
--- /dev/null
+++ b/scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci
@@ -0,0 +1,84 @@
+///
+/// Use *_pool_zalloc rather than *_pool_alloc followed by memset with 0
+///
+// Copyright: (C) 2015 Intel Corp.  GPLv2.
+// Options: --no-includes --include-headers
+//
+// Keywords: dma_pool_zalloc, pci_pool_zalloc
+//
+
+virtual context
+virtual patch
+virtual org
+virtual report
+
+//--
+//  For context mode
+//--
+
+@depends on context@
+expression x;
+statement S;
+@@
+
+* x = \(dma_pool_alloc\|pci_pool_alloc\)(...);
+  if ((x==NULL) || ...) S
+* memset(x,0, ...);
+
+//--
+//  For patch mode
+//--
+
+@depends on patch@
+expression x;
+expression a,b,c;
+statement S;
+@@
+
+- x = dma_pool_alloc(a,b,c);
++ x = dma_pool_zalloc(a,b,c);
+  if ((x==NULL) || ...) S
+- memset(x,0,...);
+
+@depends on patch@
+expression x;
+expression a,b,c;
+statement S;
+@@
+
+- x = pci_pool_alloc(a,b,c);
++ x = pci_pool_zalloc(a,b,c);
+  if ((x==NULL) || ...) S
+- memset(x,0,...);
+
+//--
+//  For org and report mode
+//--
+
+@r depends on org || report@
+expression x;
+expression a,b,c;
+statement S;
+position p;
+@@
+
+ x = @p\(dma_pool_alloc\|pci_pool_alloc\)(a,b,c);
+ if ((x==NULL) || ...) S
+ memset(x,0, ...);
+
+@script:python depends on org@
+p  r.p;
+x  r.x;
+@@
+
+msg=%s % (x)
+msg_safe=msg.replace([,@().replace(],))
+coccilib.org.print_todo(p[0], msg_safe)
+
+@script:python depends on report@
+p  r.p;
+x  r.x;
+@@
+
+msg=WARNING: *_pool_zalloc should be used for %s, instead of 
*_pool_alloc/memset % (x)
+coccilib.report.print_report(p[0], msg)
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/4] mm: add dma_pool_zalloc() pci_pool_zalloc()

[Sean O. Stalley]

Currently a call to dma_pool_alloc() with a ___GFP_ZERO flag returns
a non-zeroed memory region.

This patchset adds support for the ___GFP_ZERO flag to dma_pool_alloc(),
adds 2 wrapper functions for allocing zeroed memory from a pool, 
and provides a coccinelle script for finding  replacing instances of
dma_pool_alloc() followed by memset(0) with a single dma_pool_zalloc() call.

Sean O. Stalley (4):
  mm: Add support for __GFP_ZERO flag to dma_pool_alloc()
  mm: Add dma_pool_zalloc() call to DMA API
  pci: mm: Add pci_pool_zalloc() call
  coccinelle: mm: scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

 Documentation/DMA-API.txt  |  7 ++
 include/linux/dmapool.h|  6 ++
 include/linux/pci.h|  2 +
 mm/dmapool.c   |  6 +-
 .../coccinelle/api/alloc/pool_zalloc-simple.cocci  | 84 ++
 5 files changed, 104 insertions(+), 1 deletion(-)
 create mode 100644 scripts/coccinelle/api/alloc/pool_zalloc-simple.cocci

-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/4] pci: mm: Add pci_pool_zalloc() call

[Sean O. Stalley]

Add a wrapper function for pci_pool_alloc() to get zeroed memory.

Signed-off-by: Sean O. Stalley sean.stal...@intel.com
---
 include/linux/pci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 755a2cd..e6ec7d9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1176,6 +1176,8 @@ int pci_set_vga_state(struct pci_dev *pdev, bool decode,
dma_pool_create(name, pdev-dev, size, align, allocation)
 #definepci_pool_destroy(pool) dma_pool_destroy(pool)
 #definepci_pool_alloc(pool, flags, handle) dma_pool_alloc(pool, flags, 
handle)
+#definepci_pool_zalloc(pool, flags, handle) \
+   dma_pool_zalloc(pool, flags, handle)
 #definepci_pool_free(pool, vaddr, addr) dma_pool_free(pool, vaddr, 
addr)
 
 enum pci_dma_burst_strategy {
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/4] mm: Add dma_pool_zalloc() call to DMA API

[Sean O. Stalley]

Add a wrapper function for dma_pool_alloc() to get zeroed memory.

Signed-off-by: Sean O. Stalley sean.stal...@intel.com
---
 Documentation/DMA-API.txt | 7 +++
 include/linux/dmapool.h   | 6 ++
 2 files changed, 13 insertions(+)

diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index 5208840..988f757 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -104,6 +104,13 @@ crossing restrictions, pass 0 for alloc; passing 4096 says 
memory allocated
 from this pool must not cross 4KByte boundaries.
 
 
+   void *dma_pool_zalloc(struct dma_pool *pool, gfp_t mem_flags,
+ dma_addr_t *handle)
+
+Wraps dma_pool_alloc() and also zeroes the returned memory if the
+allocation attempt succeeded.
+
+
void *dma_pool_alloc(struct dma_pool *pool, gfp_t gfp_flags,
dma_addr_t *dma_handle);
 
diff --git a/include/linux/dmapool.h b/include/linux/dmapool.h
index 022e34f..6d8079b 100644
--- a/include/linux/dmapool.h
+++ b/include/linux/dmapool.h
@@ -22,6 +22,12 @@ void dma_pool_destroy(struct dma_pool *pool);
 void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 dma_addr_t *handle);
 
+static inline void *dma_pool_zalloc(struct dma_pool *pool, gfp_t mem_flags,
+   dma_addr_t *handle)
+{
+   return dma_pool_alloc(pool, mem_flags | __GFP_ZERO, handle);
+}
+
 void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t addr);
 
 /*
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] Read ID & Pointer from PCI Capabilities List in 1 call

[Sean O. Stalley]

Reading both fields at the same time lets us parse the
list with half the number of configspace reads.

Signed-off-by: Sean O. Stalley 
---
 drivers/pci/pci.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 5e3c8e9..db7f3d6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -145,19 +145,22 @@ static int __pci_find_next_cap_ttl(struct pci_bus *bus, 
unsigned int devfn,
   u8 pos, int cap, int *ttl)
 {
u8 id;
+   u16 ent;
+
+   pci_bus_read_config_byte(bus, devfn, pos, );
 
while ((*ttl)--) {
-   pci_bus_read_config_byte(bus, devfn, pos, );
if (pos < 0x40)
break;
pos &= ~3;
-   pci_bus_read_config_byte(bus, devfn, pos + PCI_CAP_LIST_ID,
-);
+   pci_bus_read_config_word(bus, devfn, pos, );
+
+   id = ent & 0xff;
if (id == 0xff)
break;
if (id == cap)
return pos;
-   pos += PCI_CAP_LIST_NEXT;
+   pos = (ent >> 8);
}
return 0;
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] Read ID Pointer from PCI Capabilities List in 1 call

[Sean O. Stalley]

Reading both fields at the same time lets us parse the
list with half the number of configspace reads.

Signed-off-by: Sean O. Stalley sean.stal...@intel.com
---
 drivers/pci/pci.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 5e3c8e9..db7f3d6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -145,19 +145,22 @@ static int __pci_find_next_cap_ttl(struct pci_bus *bus, 
unsigned int devfn,
   u8 pos, int cap, int *ttl)
 {
u8 id;
+   u16 ent;
+
+   pci_bus_read_config_byte(bus, devfn, pos, pos);
 
while ((*ttl)--) {
-   pci_bus_read_config_byte(bus, devfn, pos, pos);
if (pos  0x40)
break;
pos = ~3;
-   pci_bus_read_config_byte(bus, devfn, pos + PCI_CAP_LIST_ID,
-id);
+   pci_bus_read_config_word(bus, devfn, pos, ent);
+
+   id = ent  0xff;
if (id == 0xff)
break;
if (id == cap)
return pos;
-   pos += PCI_CAP_LIST_NEXT;
+   pos = (ent  8);
}
return 0;
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [V2 PATCH 01/10] added media agnostic (MA) USB HCD driver

[Sean O. Stalley]

On Wed, Nov 12, 2014 at 05:03:18PM -0500, Alan Stern wrote:
> On Wed, 12 Nov 2014, Sean O. Stalley wrote:
> > Our plan to support multiple MA devices is to have them all connected
> > to the same virtual host controller, so only 1 would be needed.
> > 
> > Would you prefer we have 1 host controller instance per MA device?
> > We are definitely open to suggestions on how this should be architected.
> 
> I haven't read the MA USB spec, so I don't know how it's intended to 
> work.  Still, what happens if you create a virtual host controller 
> with, say, 16 ports, and then someone wants to connect a 17th MA 
> device?

To summarize the spec:
MA USB groups a host & connected devices into MA service sets (MSS).
The architectural limit is 254 MA devices per MSS.

If the host needs to connect more devices than that, It can start a
new MSS and connect to 254 more MA devices.



Is supporting up to 254 devices on one machine sufficient?

Would it make sense (and does the usb stack support) having 254 root
ports on one host controller? If so, we could make our host
controller instance have 254 ports. I'm guessing the hub driver may have
a problem with this (especially for superspeed).

If that doesn't make sense (or isn't supported), we can have 1 host
controller instance per MA device. Would that be preferred?

> Also, I noticed that your patch adds a new bus type for these MA host 
> controllers.  It really seems like overkill to have a whole new bus 
> type if there's only going to be one device on it.

The bus was added when we were quickly trying to replace the platform
device code. It's probably not the right thing to do.

I'm still not sure why we can't make our hcd a platform device,
especially since dummy_hcd & the usbip's hcd are both platform devices.

> > If we get rid of these locks, endpoints can't run simultaneously.
> > MA USB IN endpoints have to copy data, which could take a while.
> 
> I don't know what you mean by "run simultaneously".  Certainly multiple 
> network packets can be transmitted and received concurrently even if 
> you use a single spinlock, since your locking won't affect the 
> networking subsystem.

I meant we couldn't have 2 threads in our driver. With one lock,
One thread would always have to wait for the other, even though
they could be working on 2 different endpoints doing completely
independent tasks.

> > Couldn't this cause a bottleneck?
> 
> Probably not enough to matter.  After all, the other host controller
> drivers rely on a single spinlock.  And if it did matter, you could
> drop the spinlock while copying the data.

Good point. We can cut our driver down to using 1 lock. If we find that
only having 1 spinlock does cause a bottleneck, we can deal with it then.


Thanks,
Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [V2 PATCH 01/10] added media agnostic (MA) USB HCD driver

[Sean O. Stalley]

On Wed, Nov 12, 2014 at 05:03:18PM -0500, Alan Stern wrote:
 On Wed, 12 Nov 2014, Sean O. Stalley wrote:
  Our plan to support multiple MA devices is to have them all connected
  to the same virtual host controller, so only 1 would be needed.
  
  Would you prefer we have 1 host controller instance per MA device?
  We are definitely open to suggestions on how this should be architected.
 
 I haven't read the MA USB spec, so I don't know how it's intended to 
 work.  Still, what happens if you create a virtual host controller 
 with, say, 16 ports, and then someone wants to connect a 17th MA 
 device?

To summarize the spec:
MA USB groups a host  connected devices into MA service sets (MSS).
The architectural limit is 254 MA devices per MSS.

If the host needs to connect more devices than that, It can start a
new MSS and connect to 254 more MA devices.



Is supporting up to 254 devices on one machine sufficient?

Would it make sense (and does the usb stack support) having 254 root
ports on one host controller? If so, we could make our host
controller instance have 254 ports. I'm guessing the hub driver may have
a problem with this (especially for superspeed).

If that doesn't make sense (or isn't supported), we can have 1 host
controller instance per MA device. Would that be preferred?

 Also, I noticed that your patch adds a new bus type for these MA host 
 controllers.  It really seems like overkill to have a whole new bus 
 type if there's only going to be one device on it.

The bus was added when we were quickly trying to replace the platform
device code. It's probably not the right thing to do.

I'm still not sure why we can't make our hcd a platform device,
especially since dummy_hcd  the usbip's hcd are both platform devices.

  If we get rid of these locks, endpoints can't run simultaneously.
  MA USB IN endpoints have to copy data, which could take a while.
 
 I don't know what you mean by run simultaneously.  Certainly multiple 
 network packets can be transmitted and received concurrently even if 
 you use a single spinlock, since your locking won't affect the 
 networking subsystem.

I meant we couldn't have 2 threads in our driver. With one lock,
One thread would always have to wait for the other, even though
they could be working on 2 different endpoints doing completely
independent tasks.

  Couldn't this cause a bottleneck?
 
 Probably not enough to matter.  After all, the other host controller
 drivers rely on a single spinlock.  And if it did matter, you could
 drop the spinlock while copying the data.

Good point. We can cut our driver down to using 1 lock. If we find that
only having 1 spinlock does cause a bottleneck, we can deal with it then.


Thanks,
Sean
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [V2 PATCH 01/10] added media agnostic (MA) USB HCD driver

[Sean O. Stalley]

Sorry, for got to respond to a couple comments. See responses below.

On Wed, Nov 12, 2014 at 01:40:21PM -0800, Sean O. Stalley wrote:
> Thanks for reviewing. My responses are inline.
> 
> Greg has asked that we clean up this code internally before we
> send out another patchset to the mailing list. I will address
> the issues you pointed out, but it may be a while before you see
> another patchset.
> 
> Thanks Again,
> Sean
> 
> On Tue, Nov 11, 2014 at 10:54:30AM -0500, Alan Stern wrote:
> > On Mon, 10 Nov 2014, Stephanie Wallick wrote:
> > > + /* If the endpoint isn't activated, we can't enqueue anything. */
> > > + if (MAUSB_EP_HANDLE_UNASSIGNED == ep->ep_handle_state) {
> > > + mausb_err(, "%s: endpoint handle unassigned\n", __func__);
> > > + return -EPIPE;
> > > + }
> > > +
> > > + if (USB_SPEED_FULL != urb->dev->speed) /* suppress checks */
> > > + ep->max_pkt = usb_endpoint_maxp(>ep->desc);
> > 
> > What happens to full-speed devices?  Don't they have maxpacket values?
> > 

This was part of a work-around.
Per the MA spec (section 7.3.2.2), wMaxPacketSize should be initially
set to 8 for EP0 of FS devices. The usbcore sets it to 64.

This makes sure the EPHandleReq Packet has the per-spec values.

> > > + if (ret < 0) {
> > > + mausb_err(, "urb enqueue failed: error %d\n", ret);
> > > + usb_hcd_unlink_urb_from_ep(hcd, urb);
> > > + return ret;
> > > + }
> > > +
> > > + /* get usb device and increment reference counter */
> > > + if (!mhcd.udev) {
> > > + mhcd.udev = urb->dev;
> > > + usb_get_dev(mhcd.udev);
> > > + }
> > 
> > What happens if more than one device is in use at a time?
> > 

This is wrong. This call should be in mausb_internal_alloc_dev(). Will fix.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [V2 PATCH 01/10] added media agnostic (MA) USB HCD driver

[Sean O. Stalley]

Thanks for reviewing. My responses are inline.

Greg has asked that we clean up this code internally before we
send out another patchset to the mailing list. I will address
the issues you pointed out, but it may be a while before you see
another patchset.

Thanks Again,
Sean

On Tue, Nov 11, 2014 at 10:54:30AM -0500, Alan Stern wrote:
> On Mon, 10 Nov 2014, Stephanie Wallick wrote:
> 
> > +static struct mausb_hcd mhcd;
> 
> Only one statically-allocated structure?  What if somebody wants to 
> have more than one of these things in their system?
> 

Our plan to support multiple MA devices is to have them all connected
to the same virtual host controller, so only 1 would be needed.

Would you prefer we have 1 host controller instance per MA device?
We are definitely open to suggestions on how this should be architected.

> > +/**
> > + * @maurb: Media agnostic structure with URB to release.
> > + * @status:Status for URB that is getting released.
> > + *
> > + * Removes an URB from the queue, deletes the media agnostic information in
> > + * the urb, and gives the URB back to the HCD. Caller must be holding the
> > + * driver's spinlock.
> > + */
> > +void mausb_unlink_giveback_urb(struct mausb_urb *maurb, int status)
> > +{
> > +   struct urb  *urb;
> > +   struct usb_hcd  *hcd;
> > +   struct api_context  *ctx = NULL;
> > +   unsigned long   irq_flags;
> > +
> > +   hcd = mausb_hcd_to_usb_hcd();
> > +
> > +   spin_lock_irqsave(_lock, irq_flags);
> 
> Why do you need multiple spinlocks?  Isn't one lock sufficient?
> 
We will simplify the locking scheme before resubmitting.

I think it might be worthwhile to have a per-endpoint lock, see below.

> > +   if (!maurb) {
> > +   mausb_err(, "%s: no maurb\n", __func__);
> > +   spin_unlock_irqrestore(_lock, irq_flags);
> > +   return;
> > +   } else {
> > +   urb = maurb->urb;
> > +   ctx = urb->context;
> > +   }
> > +
> > +   if (!urb) {
> > +   mausb_err(, "%s: no urb\n", __func__);
> > +   mausb_internal_drop_maurb(maurb, );
> > +   spin_unlock_irqrestore(_lock, irq_flags);
> > +   return;
> > +   }
> > +
> > +   mausb_dbg(, "%s: returning urb with status %i\n", __func__, 
> > status);
> > +
> > +   usb_hcd_unlink_urb_from_ep(hcd, urb);
> > +   usb_hcd_giveback_urb(hcd, urb, status);
> 
> You must not call this function while holding any spinlocks.  What happens
> if the URB's completion routine tries to resubmit?
> 

This works with our multi-lock scheme, but I will fix when we move to 1 lock.

> > +
> > +   /* remove the mausb-specific data */
> > +   mausb_internal_drop_maurb(maurb, );
> > +
> > +   spin_unlock_irqrestore(_lock, irq_flags);
> > +}
> > +
> > +/**
> > + * Adds an URB to the endpoint queue then calls the URB handler. URB is 
> > wrapped
> > + * in media agnostic structure before being enqueued.
> > + */
> > +static int mausb_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
> > +   gfp_t memflags)
> > +{
> > +   int ret = 0;
> > +   struct mausb_urb*maurb;
> > +   struct mausb_host_ep*ep;
> > +   unsigned long   irq_flags;
> > +
> > +   if (!hcd || !urb) {
> > +   pr_err("%s: no %s\n", __func__, (hcd ? "urb" : "USB hcd"));
> > +   }
> 
> This can never happen.  The USB core guarantees it; you don't need 
> to check.
> 

I will remove this check (along with any other unnecessary checks for things
guaranteed from usbcore).

> > +   ep   = usb_to_ma_endpoint(urb->ep);
> > +
> > +   if (!ep) {
> > +   mausb_err(, "%s: no endpoint\n", __func__);
> > +   return -EINVAL;
> > +   }
> > +
> > +   if (urb->status != -EINPROGRESS) {
> > +   mausb_err(, "%s: urb already unlinked, status is %i\n",
> > +   __func__, urb->status);
> > +   return urb->status;
> > +   }
> 
> You also don't need to check this.
> 
Will remove.

> > +   /* If the endpoint isn't activated, we can't enqueue anything. */
> > +   if (MAUSB_EP_HANDLE_UNASSIGNED == ep->ep_handle_state) {
> > +   mausb_err(, "%s: endpoint handle unassigned\n", __func__);
> > +   return -EPIPE;
> > +   }
> > +
> > +   if (USB_SPEED_FULL != urb->dev->speed) /* suppress checks */
> > +   ep->max_pkt = usb_endpoint_maxp(>ep->desc);
> 
> What happens to full-speed devices?  Don't they have maxpacket values?
> 
> > +
> > +   /* initialize the maurb */
> > +   maurb = mausb_alloc_maurb(ep, memflags);
> > +   if (!maurb) {
> > +   mausb_err(, "could not allocate memory for MA USB urb\n");
> > +   return -ENOMEM;
> > +   }
> > +
> > +   /* set maurb member values */
> > +   maurb->urb = urb;
> > +   urb->hcpriv = maurb;
> > +
> > +   /* submit urb to hcd and add to endpoint queue */
> > +   ret = usb_hcd_link_urb_to_ep(hcd, urb);
> 
> Read the kerneldoc for this function.  You must hold your private
> spinlock when you call it.
> 

Will fix this & make sure 

Re: [PATCH 05/10] added media specific (MS) TCP drivers

[Sean O. Stalley]

Thank You for reviewing our code.

I believe most of the problems you pointed out in mausb_ioctl.c
were addressed in [V2 PATCH 5/10]. I am working on adding the proper
error checking to the TCP drivers.

Greg has requested that we clean up our code internally before
submitting another patchset to the mailing list. I will make sure
we fix the problems you pointed out, but it may be a while before
you see another patchset.

Thanks,
Sean

On Tue, Nov 04, 2014 at 09:48:33AM +0100, Tobias Klauser wrote:
> On 2014-11-03 at 21:42:52 +0100, Stephanie Wallick 
>  wrote:
> > This is where we handle media specific packets and transport. The MS driver
> > interfaces with a media agnostic (MA) driver via a series of transfer pairs.
> > Transfer pairs consist of a set of functions to pass MA USB packets back
> > and forth between MA and MS drivers. There is one transfer pair per device
> > endpoint and one transfer pair for control/management traffic. When the MA
> > driver needs to send an MA USB packet, it hands the packet off to the MS
> > layer where the packet is converted into an MS form and sent via TCP over
> > the underlying ethernet or wireless medium. When the MS driver receives a
> > packet, it converts it into an MA USB packet and hands it off the the MA
> > driver for handling.
> > 
> > In addition, the MS driver provides an interface to inititate connection 
> > events.
> > Because there are no physical MA USB ports in an MA USB host, the host must 
> > be
> > notified via software when a device is connected.
> > 
> > Lastly, the MS driver contains a number of ioctl functions that are used by 
> > a
> > utility to adjust medium-related driver parameters and connect or 
> > disconnect the
> > MA USB host and device drivers.
> > 
> > Signed-off-by: Sean O. Stalley 
> > Signed-off-by: Stephanie Wallick 
> > ---
> >  drivers/staging/mausb/drivers/mausb_ioctl.c  | 373 +++
> >  drivers/staging/mausb/drivers/mausb_ioctl.h  |  99 +
> >  drivers/staging/mausb/drivers/mausb_msapi.c  | 110 ++
> >  drivers/staging/mausb/drivers/mausb_msapi.h  | 232 
> >  drivers/staging/mausb/drivers/mausb_tcp-device.c | 147 
> >  drivers/staging/mausb/drivers/mausb_tcp-host.c   | 144 
> >  drivers/staging/mausb/drivers/mausb_tcp.c| 446 
> > +++
> >  drivers/staging/mausb/drivers/mausb_tcp.h| 129 +++
> >  8 files changed, 1680 insertions(+)
> >  create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.c
> >  create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.h
> >  create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.c
> >  create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.h
> >  create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-device.c
> >  create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-host.c
> >  create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.c
> >  create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.h
> > 
> > diff --git a/drivers/staging/mausb/drivers/mausb_ioctl.c 
> > b/drivers/staging/mausb/drivers/mausb_ioctl.c
> > new file mode 100644
> > index 000..0c6c6bd
> > --- /dev/null
> > +++ b/drivers/staging/mausb/drivers/mausb_ioctl.c
> 
> [...]
> 
> > +/**
> > + * This function is used to send a message to the user, in other words, the
> > + * calling process. It basically copies the message one byte at a time.
> > + *
> > + * @msg:   The message to be sent to the user.
> > + * @buffer:The buffer in which to put the message. This buffer was 
> > given to
> > + * us to fill.
> > + */
> > +void to_user(char *msg, long unsigned int buffer)
> > +{
> > +   int length = (int)strlen(msg);
> > +   int bytes = 0;
> > +
> > +   while (length && *msg) {
> > +   put_user(*(msg++), (char *)buffer++);
> > +   length--;
> > +   bytes++;
> > +   }
> 
> Any reason not to use copy_to_user here? That way, access_ok would only
> need to be executed once for the whole range.
> 
> In any case, the return value of put_user/copy_to_user will need to be
> checked.
> 
> > +
> > +   put_user('\0', (char *)buffer + bytes);
> > +}
> 
> [...]
> 
> > +/**
> > + * This function is used to read from the device file. From the 
> > perspective of
> > + * the device, the user is reading information from us. This is one of the
> > + * entry points to this module.
> > + *
> > + * @file:  The device file. We don't us

Re: [V2 PATCH 02/10] added media agnostic (MA) USB HCD roothubs

[Sean O. Stalley]

Thank you for your review. My responses are inline.

Greg has requested that we clean up the driver internally before
we resubmit another patchset to the mailing list. I will make
sure the changes you requested make it in, but it may be a while
before you see a patchset with the fixes included.

Thanks,
Sean O. Stalley

On Wed, Nov 12, 2014 at 09:35:42AM +0100, Oliver Neukum wrote:
> On Mon, 2014-11-10 at 18:09 -0800, Stephanie Wallick wrote:
> > diff --git a/drivers/staging/mausb/drivers/mausb_hub.c 
> > b/drivers/staging/mausb/drivers/mausb_hub.c
> > new file mode 100644
> > index 000..63c0fe4
> > --- /dev/null
> > +++ b/drivers/staging/mausb/drivers/mausb_hub.c
> 
> > +/**
> > + * Returns true if the given is the superspeed HCD. Note: The primary HCD 
> > is
> > + * High Speed and the shared HCD is SuperSpeed.
> > + */
> 
> Why in that order?
> 

We should probably switch this & make the superspeed hub primary.
That way we match the xhci driver.

> > +bool mausb_is_ss_hcd(struct usb_hcd *hcd)
> > +{
> > +   if (usb_hcd_is_primary_hcd(hcd))
> > +   return false;
> > +   else
> > +   return true;
> > +}
> 
> 
> 
> > +
> > +/**
> > + * Called by usb core when polling for a port status change.
> > + *
> > + * @hcd:   USB HCD being polled.
> > + * @buf:   Holds port status changes (if any).
> > + *
> > + * Returns zero if there is no status change, otherwise returns number of
> > + * bytes in buf. When there is a status change on a port, the bit indexed
> > + * at the port number + 1 (e.g. bit 2 for port 1) is set in the buffer.
> > + */
> > +int mausb_hub_status_data(struct usb_hcd *hcd, char *buf)
> > +{
> > +   int  i;
> > +   u16  port_change = 0;
> > +   u32  status = 0;
> > +   int  ret = 1;
> > +   struct mausb_hcd *mhcd = usb_hcd_to_mausb_hcd(hcd);
> > +   struct mausb_root_hub*roothub = usb_hcd_to_roothub(hcd);
> > +
> > +   /*
> > +* Buf should never be more that 2 bytes. USB 3.0 hubs cannot have
> > +* more than 15 downstream ports.
> > +*/
> > +   buf[0] = 0;
> > +   if (MAUSB_ROOTHUB_NUM_PORTS > 7) {
> > +   buf[1] = 0;
> > +   ret++;
> > +   }
> 
> Endianness bug.
> 

Could you elaborate?
It was my understanding that this buffer was host-endian.
Is this an unacceptable way to clear the buffer?

> > +
> > +   for (i = 0; i < MAUSB_ROOTHUB_NUM_PORTS; i++) {
> > +   port_change = roothub->port_status[i].wPortChange;
> > +   if (port_change)
> > +   status |= (1 << (i + 1));
> > +   }
> > +
> > +   mausb_dbg(mhcd, "%s: hub status is 0x%x\n", __func__, status);
> > +
> > +   /* hcd might be suspended, resume if there is a status change */
> > +   if (mhcd->disabled == 0) {
> > +   if ((hcd->state == HC_STATE_SUSPENDED) && status)
> > +   usb_hcd_resume_root_hub(hcd);
> > +   }
> > +
> > +   memcpy(buf, (char *), ret);
> > +
> > +   return status ? ret : 0;
> > +}
> > +
> > +/**
> > + * Sets the bitfields in the hub descriptor of the 2.0 root hub. Always
> > + * returns zero.
> > + */
> > +int mausb_set_hub_descriptor(struct usb_hub_descriptor *hub_des)
> > +{
> > +   /* set the values to the default */
> > +   hub_des->bDescLength  = sizeof(struct usb_hub_descriptor);
> > +   hub_des->bDescriptorType  = USB_DT_HUB;
> > +   hub_des->bNbrPorts= MAUSB_ROOTHUB_NUM_PORTS;
> > +   hub_des->wHubCharacteristics  = MAUSB_ROOTHUB_CHAR;
> > +   hub_des->bPwrOn2PwrGood   = MAUSB_ROOTHUB_PWR_ON_2_PWR_GOOD;
> > +   hub_des->bHubContrCurrent = MAUSB_ROOTHUB_CONTR_CURRENT;
> 
> Is that descriptor in bus or host endianness?
> 

All of the fields are little-endian. We should be using cpu_to_le16()
when setting wHubCharacteristics.

> > +
> > +   return 0;
> > +}
> > +
> > +/**
> > + * Sets the bitfields in the hub descriptor of the 3.0 root hub. Always
> > + * returns zero.
> 
> Then why return anything?
> 

Good point. I will change this (and mausb_set_hub_descriptor()) to be void.

> > + */
> > +int mausb_set_ss_hub_descriptor(struct usb_hub_descriptor *hub_des)
> > +{
> > +   /* set the values to the default */
> > +   hub_des->bDescLength  = sizeof(struct usb_hub_descriptor);
> > +   hub_des->bDescriptorType  

Re: [V2 PATCH 02/10] added media agnostic (MA) USB HCD roothubs

[Sean O. Stalley]

Thank you for your review. My responses are inline.

Greg has requested that we clean up the driver internally before
we resubmit another patchset to the mailing list. I will make
sure the changes you requested make it in, but it may be a while
before you see a patchset with the fixes included.

Thanks,
Sean O. Stalley

On Wed, Nov 12, 2014 at 09:35:42AM +0100, Oliver Neukum wrote:
 On Mon, 2014-11-10 at 18:09 -0800, Stephanie Wallick wrote:
  diff --git a/drivers/staging/mausb/drivers/mausb_hub.c 
  b/drivers/staging/mausb/drivers/mausb_hub.c
  new file mode 100644
  index 000..63c0fe4
  --- /dev/null
  +++ b/drivers/staging/mausb/drivers/mausb_hub.c
 
  +/**
  + * Returns true if the given is the superspeed HCD. Note: The primary HCD 
  is
  + * High Speed and the shared HCD is SuperSpeed.
  + */
 
 Why in that order?
 

We should probably switch this  make the superspeed hub primary.
That way we match the xhci driver.

  +bool mausb_is_ss_hcd(struct usb_hcd *hcd)
  +{
  +   if (usb_hcd_is_primary_hcd(hcd))
  +   return false;
  +   else
  +   return true;
  +}
 
 
 
  +
  +/**
  + * Called by usb core when polling for a port status change.
  + *
  + * @hcd:   USB HCD being polled.
  + * @buf:   Holds port status changes (if any).
  + *
  + * Returns zero if there is no status change, otherwise returns number of
  + * bytes in buf. When there is a status change on a port, the bit indexed
  + * at the port number + 1 (e.g. bit 2 for port 1) is set in the buffer.
  + */
  +int mausb_hub_status_data(struct usb_hcd *hcd, char *buf)
  +{
  +   int  i;
  +   u16  port_change = 0;
  +   u32  status = 0;
  +   int  ret = 1;
  +   struct mausb_hcd *mhcd = usb_hcd_to_mausb_hcd(hcd);
  +   struct mausb_root_hub*roothub = usb_hcd_to_roothub(hcd);
  +
  +   /*
  +* Buf should never be more that 2 bytes. USB 3.0 hubs cannot have
  +* more than 15 downstream ports.
  +*/
  +   buf[0] = 0;
  +   if (MAUSB_ROOTHUB_NUM_PORTS  7) {
  +   buf[1] = 0;
  +   ret++;
  +   }
 
 Endianness bug.
 

Could you elaborate?
It was my understanding that this buffer was host-endian.
Is this an unacceptable way to clear the buffer?

  +
  +   for (i = 0; i  MAUSB_ROOTHUB_NUM_PORTS; i++) {
  +   port_change = roothub-port_status[i].wPortChange;
  +   if (port_change)
  +   status |= (1  (i + 1));
  +   }
  +
  +   mausb_dbg(mhcd, %s: hub status is 0x%x\n, __func__, status);
  +
  +   /* hcd might be suspended, resume if there is a status change */
  +   if (mhcd-disabled == 0) {
  +   if ((hcd-state == HC_STATE_SUSPENDED)  status)
  +   usb_hcd_resume_root_hub(hcd);
  +   }
  +
  +   memcpy(buf, (char *)status, ret);
  +
  +   return status ? ret : 0;
  +}
  +
  +/**
  + * Sets the bitfields in the hub descriptor of the 2.0 root hub. Always
  + * returns zero.
  + */
  +int mausb_set_hub_descriptor(struct usb_hub_descriptor *hub_des)
  +{
  +   /* set the values to the default */
  +   hub_des-bDescLength  = sizeof(struct usb_hub_descriptor);
  +   hub_des-bDescriptorType  = USB_DT_HUB;
  +   hub_des-bNbrPorts= MAUSB_ROOTHUB_NUM_PORTS;
  +   hub_des-wHubCharacteristics  = MAUSB_ROOTHUB_CHAR;
  +   hub_des-bPwrOn2PwrGood   = MAUSB_ROOTHUB_PWR_ON_2_PWR_GOOD;
  +   hub_des-bHubContrCurrent = MAUSB_ROOTHUB_CONTR_CURRENT;
 
 Is that descriptor in bus or host endianness?
 

All of the fields are little-endian. We should be using cpu_to_le16()
when setting wHubCharacteristics.

  +
  +   return 0;
  +}
  +
  +/**
  + * Sets the bitfields in the hub descriptor of the 3.0 root hub. Always
  + * returns zero.
 
 Then why return anything?
 

Good point. I will change this (and mausb_set_hub_descriptor()) to be void.

  + */
  +int mausb_set_ss_hub_descriptor(struct usb_hub_descriptor *hub_des)
  +{
  +   /* set the values to the default */
  +   hub_des-bDescLength  = sizeof(struct usb_hub_descriptor);
  +   hub_des-bDescriptorType  = USB_DT_SS_HUB;
  +   hub_des-bNbrPorts= MAUSB_ROOTHUB_NUM_PORTS;
  +   hub_des-wHubCharacteristics  = MAUSB_ROOTHUB_CHAR;
  +   hub_des-bPwrOn2PwrGood   = MAUSB_ROOTHUB_PWR_ON_2_PWR_GOOD;
  +   hub_des-bHubContrCurrent = MAUSB_ROOTHUB_CONTR_CURRENT;
  +
  +   /* USB3-specific parameters */
  +   hub_des-u.ss.bHubHdrDecLat   = MAUSB_ROOTHUB_HDR_DEC_LAT;
  +   hub_des-u.ss.wHubDelay   = MAUSB_ROOTHUB_DELAY;
  +   hub_des-u.ss.DeviceRemovable = MAUSB_ALL_DEV_REMOVABLE;
  +
  +   return 0;
  +}
 
 
  +/**
  + * Contains all the structures required to emulate a root hub. One instance
  + * exists per root hub.
  + */
  +struct __attribute__((__packed__)) mausb_root_hub {
 
 Why __packed__ ?

Doesn't need to be. I will remove.

  +
  +   /* hub parameters */
  +   struct usb_hub_descriptor descriptor;
  +   struct usb_hub_status status;
  +
  +   /* port

Re: [PATCH 05/10] added media specific (MS) TCP drivers

[Sean O. Stalley]

Thank You for reviewing our code.

I believe most of the problems you pointed out in mausb_ioctl.c
were addressed in [V2 PATCH 5/10]. I am working on adding the proper
error checking to the TCP drivers.

Greg has requested that we clean up our code internally before
submitting another patchset to the mailing list. I will make sure
we fix the problems you pointed out, but it may be a while before
you see another patchset.

Thanks,
Sean

On Tue, Nov 04, 2014 at 09:48:33AM +0100, Tobias Klauser wrote:
 On 2014-11-03 at 21:42:52 +0100, Stephanie Wallick 
 stephanie.s.wall...@intel.com wrote:
  This is where we handle media specific packets and transport. The MS driver
  interfaces with a media agnostic (MA) driver via a series of transfer pairs.
  Transfer pairs consist of a set of functions to pass MA USB packets back
  and forth between MA and MS drivers. There is one transfer pair per device
  endpoint and one transfer pair for control/management traffic. When the MA
  driver needs to send an MA USB packet, it hands the packet off to the MS
  layer where the packet is converted into an MS form and sent via TCP over
  the underlying ethernet or wireless medium. When the MS driver receives a
  packet, it converts it into an MA USB packet and hands it off the the MA
  driver for handling.
  
  In addition, the MS driver provides an interface to inititate connection 
  events.
  Because there are no physical MA USB ports in an MA USB host, the host must 
  be
  notified via software when a device is connected.
  
  Lastly, the MS driver contains a number of ioctl functions that are used by 
  a
  utility to adjust medium-related driver parameters and connect or 
  disconnect the
  MA USB host and device drivers.
  
  Signed-off-by: Sean O. Stalley sean.stal...@intel.com
  Signed-off-by: Stephanie Wallick stephanie.s.wall...@intel.com
  ---
   drivers/staging/mausb/drivers/mausb_ioctl.c  | 373 +++
   drivers/staging/mausb/drivers/mausb_ioctl.h  |  99 +
   drivers/staging/mausb/drivers/mausb_msapi.c  | 110 ++
   drivers/staging/mausb/drivers/mausb_msapi.h  | 232 
   drivers/staging/mausb/drivers/mausb_tcp-device.c | 147 
   drivers/staging/mausb/drivers/mausb_tcp-host.c   | 144 
   drivers/staging/mausb/drivers/mausb_tcp.c| 446 
  +++
   drivers/staging/mausb/drivers/mausb_tcp.h| 129 +++
   8 files changed, 1680 insertions(+)
   create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.c
   create mode 100644 drivers/staging/mausb/drivers/mausb_ioctl.h
   create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.c
   create mode 100644 drivers/staging/mausb/drivers/mausb_msapi.h
   create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-device.c
   create mode 100644 drivers/staging/mausb/drivers/mausb_tcp-host.c
   create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.c
   create mode 100644 drivers/staging/mausb/drivers/mausb_tcp.h
  
  diff --git a/drivers/staging/mausb/drivers/mausb_ioctl.c 
  b/drivers/staging/mausb/drivers/mausb_ioctl.c
  new file mode 100644
  index 000..0c6c6bd
  --- /dev/null
  +++ b/drivers/staging/mausb/drivers/mausb_ioctl.c
 
 [...]
 
  +/**
  + * This function is used to send a message to the user, in other words, the
  + * calling process. It basically copies the message one byte at a time.
  + *
  + * @msg:   The message to be sent to the user.
  + * @buffer:The buffer in which to put the message. This buffer was 
  given to
  + * us to fill.
  + */
  +void to_user(char *msg, long unsigned int buffer)
  +{
  +   int length = (int)strlen(msg);
  +   int bytes = 0;
  +
  +   while (length  *msg) {
  +   put_user(*(msg++), (char *)buffer++);
  +   length--;
  +   bytes++;
  +   }
 
 Any reason not to use copy_to_user here? That way, access_ok would only
 need to be executed once for the whole range.
 
 In any case, the return value of put_user/copy_to_user will need to be
 checked.
 
  +
  +   put_user('\0', (char *)buffer + bytes);
  +}
 
 [...]
 
  +/**
  + * This function is used to read from the device file. From the 
  perspective of
  + * the device, the user is reading information from us. This is one of the
  + * entry points to this module.
  + *
  + * @file:  The device file. We don't use it directly, but it's passed in.
  + * @buffer:The buffer to put the message into.
  + * @length:The max length to be read.
  + * @offset:File offset, which we don't use but it is passed in 
  nontheless.
  + */
  +static ssize_t mausb_read(struct file *file, char __user *buffer,
  +   size_t length, loff_t *offset)
  +{
  +   int bytes_read = 0;
  +
  +   if (*message_point == 0)
  +   return 0;
  +   while (length  *message_point) {
  +   put_user(*(message_point++), buffer++);
  +   length--;
  +   bytes_read++;
  +   }
 
 See comment for to_user above. Why

Re: [V2 PATCH 01/10] added media agnostic (MA) USB HCD driver

[Sean O. Stalley]

Thanks for reviewing. My responses are inline.

Greg has asked that we clean up this code internally before we
send out another patchset to the mailing list. I will address
the issues you pointed out, but it may be a while before you see
another patchset.

Thanks Again,
Sean

On Tue, Nov 11, 2014 at 10:54:30AM -0500, Alan Stern wrote:
 On Mon, 10 Nov 2014, Stephanie Wallick wrote:
 
  +static struct mausb_hcd mhcd;
 
 Only one statically-allocated structure?  What if somebody wants to 
 have more than one of these things in their system?
 

Our plan to support multiple MA devices is to have them all connected
to the same virtual host controller, so only 1 would be needed.

Would you prefer we have 1 host controller instance per MA device?
We are definitely open to suggestions on how this should be architected.

  +/**
  + * @maurb: Media agnostic structure with URB to release.
  + * @status:Status for URB that is getting released.
  + *
  + * Removes an URB from the queue, deletes the media agnostic information in
  + * the urb, and gives the URB back to the HCD. Caller must be holding the
  + * driver's spinlock.
  + */
  +void mausb_unlink_giveback_urb(struct mausb_urb *maurb, int status)
  +{
  +   struct urb  *urb;
  +   struct usb_hcd  *hcd;
  +   struct api_context  *ctx = NULL;
  +   unsigned long   irq_flags;
  +
  +   hcd = mausb_hcd_to_usb_hcd(mhcd);
  +
  +   spin_lock_irqsave(mhcd.giveback_lock, irq_flags);
 
 Why do you need multiple spinlocks?  Isn't one lock sufficient?
 
We will simplify the locking scheme before resubmitting.

I think it might be worthwhile to have a per-endpoint lock, see below.

  +   if (!maurb) {
  +   mausb_err(mhcd, %s: no maurb\n, __func__);
  +   spin_unlock_irqrestore(mhcd.giveback_lock, irq_flags);
  +   return;
  +   } else {
  +   urb = maurb-urb;
  +   ctx = urb-context;
  +   }
  +
  +   if (!urb) {
  +   mausb_err(mhcd, %s: no urb\n, __func__);
  +   mausb_internal_drop_maurb(maurb, mhcd);
  +   spin_unlock_irqrestore(mhcd.giveback_lock, irq_flags);
  +   return;
  +   }
  +
  +   mausb_dbg(mhcd, %s: returning urb with status %i\n, __func__, 
  status);
  +
  +   usb_hcd_unlink_urb_from_ep(hcd, urb);
  +   usb_hcd_giveback_urb(hcd, urb, status);
 
 You must not call this function while holding any spinlocks.  What happens
 if the URB's completion routine tries to resubmit?
 

This works with our multi-lock scheme, but I will fix when we move to 1 lock.

  +
  +   /* remove the mausb-specific data */
  +   mausb_internal_drop_maurb(maurb, mhcd);
  +
  +   spin_unlock_irqrestore(mhcd.giveback_lock, irq_flags);
  +}
  +
  +/**
  + * Adds an URB to the endpoint queue then calls the URB handler. URB is 
  wrapped
  + * in media agnostic structure before being enqueued.
  + */
  +static int mausb_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
  +   gfp_t memflags)
  +{
  +   int ret = 0;
  +   struct mausb_urb*maurb;
  +   struct mausb_host_ep*ep;
  +   unsigned long   irq_flags;
  +
  +   if (!hcd || !urb) {
  +   pr_err(%s: no %s\n, __func__, (hcd ? urb : USB hcd));
  +   }
 
 This can never happen.  The USB core guarantees it; you don't need 
 to check.
 

I will remove this check (along with any other unnecessary checks for things
guaranteed from usbcore).

  +   ep   = usb_to_ma_endpoint(urb-ep);
  +
  +   if (!ep) {
  +   mausb_err(mhcd, %s: no endpoint\n, __func__);
  +   return -EINVAL;
  +   }
  +
  +   if (urb-status != -EINPROGRESS) {
  +   mausb_err(mhcd, %s: urb already unlinked, status is %i\n,
  +   __func__, urb-status);
  +   return urb-status;
  +   }
 
 You also don't need to check this.
 
Will remove.

  +   /* If the endpoint isn't activated, we can't enqueue anything. */
  +   if (MAUSB_EP_HANDLE_UNASSIGNED == ep-ep_handle_state) {
  +   mausb_err(mhcd, %s: endpoint handle unassigned\n, __func__);
  +   return -EPIPE;
  +   }
  +
  +   if (USB_SPEED_FULL != urb-dev-speed) /* suppress checks */
  +   ep-max_pkt = usb_endpoint_maxp(urb-ep-desc);
 
 What happens to full-speed devices?  Don't they have maxpacket values?
 
  +
  +   /* initialize the maurb */
  +   maurb = mausb_alloc_maurb(ep, memflags);
  +   if (!maurb) {
  +   mausb_err(mhcd, could not allocate memory for MA USB urb\n);
  +   return -ENOMEM;
  +   }
  +
  +   /* set maurb member values */
  +   maurb-urb = urb;
  +   urb-hcpriv = maurb;
  +
  +   /* submit urb to hcd and add to endpoint queue */
  +   ret = usb_hcd_link_urb_to_ep(hcd, urb);
 
 Read the kerneldoc for this function.  You must hold your private
 spinlock when you call it.
 

Will fix this  make sure we hold our lock.

  +   if (ret  0) {
  +   mausb_err(mhcd, urb enqueue failed: error %d\n, ret);
  +   usb_hcd_unlink_urb_from_ep(hcd, urb);
  

Re: [V2 PATCH 01/10] added media agnostic (MA) USB HCD driver

[Sean O. Stalley]

Sorry, for got to respond to a couple comments. See responses below.

On Wed, Nov 12, 2014 at 01:40:21PM -0800, Sean O. Stalley wrote:
 Thanks for reviewing. My responses are inline.
 
 Greg has asked that we clean up this code internally before we
 send out another patchset to the mailing list. I will address
 the issues you pointed out, but it may be a while before you see
 another patchset.
 
 Thanks Again,
 Sean
 
 On Tue, Nov 11, 2014 at 10:54:30AM -0500, Alan Stern wrote:
  On Mon, 10 Nov 2014, Stephanie Wallick wrote:
   + /* If the endpoint isn't activated, we can't enqueue anything. */
   + if (MAUSB_EP_HANDLE_UNASSIGNED == ep-ep_handle_state) {
   + mausb_err(mhcd, %s: endpoint handle unassigned\n, __func__);
   + return -EPIPE;
   + }
   +
   + if (USB_SPEED_FULL != urb-dev-speed) /* suppress checks */
   + ep-max_pkt = usb_endpoint_maxp(urb-ep-desc);
  
  What happens to full-speed devices?  Don't they have maxpacket values?
  

This was part of a work-around.
Per the MA spec (section 7.3.2.2), wMaxPacketSize should be initially
set to 8 for EP0 of FS devices. The usbcore sets it to 64.

This makes sure the EPHandleReq Packet has the per-spec values.

   + if (ret  0) {
   + mausb_err(mhcd, urb enqueue failed: error %d\n, ret);
   + usb_hcd_unlink_urb_from_ep(hcd, urb);
   + return ret;
   + }
   +
   + /* get usb device and increment reference counter */
   + if (!mhcd.udev) {
   + mhcd.udev = urb-dev;
   + usb_get_dev(mhcd.udev);
   + }
  
  What happens if more than one device is in use at a time?
  

This is wrong. This call should be in mausb_internal_alloc_dev(). Will fix.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [V2 PATCH 03/10] added media agnostic (MA) data structures and handling

[Sean O. Stalley]

On Tue, Nov 11, 2014 at 01:38:21PM +0900, Greg KH wrote:
> On Mon, Nov 10, 2014 at 06:09:34PM -0800, Stephanie Wallick wrote:
> Intel has a whole group of very experienced Linux kernel developers who
> will review code before you sent it out publicly.  Please take advantage
> of them and run this all through them before resending this out again.
> 
> If you did run this code through that group, please let me know who it
> was specifically that allowed this stuff to get through, and why they
> didn't want their name on this code submission.  I need to have a strong
> word with them...

We submitted the patches for internal review and got no objections to
release. We will be more aggressive in seeking out feedback (and approval)
before resubmitting any code.
 
> Yes, I am holding you to a higher standard than staging code normally
> is, and yes, it is purely because of the company you work for.  But I
> only do that because your company knows how to do this stuff right, and
> you have access to the resources and talent to help make this code
> right.  Other people and companies do not have the kind of advantage
> that you do.

We know we are fortunate to work for a company with so much talent and
resources and we don't mind being held to a higher standard. We have been
receiving multiple requests for our host driver and wanted to make it
available as soon as possible for others to use. We thought putting our
host driver into staging would be a good way to release it, but realize now
that it was premature. 

> Wasting community member's time (i.e. mine) by forcing _them_ to review
> stuff like this, is something that your company knows better than to do,
> as should you as well.
> 
> I want to see some more senior Intel kernel developer's signed-off-by
> lines on this code before I will ever consider accepting it for the
> kernel.  Please do not resend this code until that happens.
> 
> greg k-h

We apologize for wasting everyone's time and will certainly learn from this.
We won't resubmit the driver until a senior kernel developer has signed off on 
it.

Sincerely,
Sean O. Stalley
Stephanie S. Wallick
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [V2 PATCH 03/10] added media agnostic (MA) data structures and handling

[Sean O. Stalley]

On Tue, Nov 11, 2014 at 01:38:21PM +0900, Greg KH wrote:
 On Mon, Nov 10, 2014 at 06:09:34PM -0800, Stephanie Wallick wrote:
 Intel has a whole group of very experienced Linux kernel developers who
 will review code before you sent it out publicly.  Please take advantage
 of them and run this all through them before resending this out again.
 
 If you did run this code through that group, please let me know who it
 was specifically that allowed this stuff to get through, and why they
 didn't want their name on this code submission.  I need to have a strong
 word with them...

We submitted the patches for internal review and got no objections to
release. We will be more aggressive in seeking out feedback (and approval)
before resubmitting any code.
 
 Yes, I am holding you to a higher standard than staging code normally
 is, and yes, it is purely because of the company you work for.  But I
 only do that because your company knows how to do this stuff right, and
 you have access to the resources and talent to help make this code
 right.  Other people and companies do not have the kind of advantage
 that you do.

We know we are fortunate to work for a company with so much talent and
resources and we don't mind being held to a higher standard. We have been
receiving multiple requests for our host driver and wanted to make it
available as soon as possible for others to use. We thought putting our
host driver into staging would be a good way to release it, but realize now
that it was premature. 

 Wasting community member's time (i.e. mine) by forcing _them_ to review
 stuff like this, is something that your company knows better than to do,
 as should you as well.
 
 I want to see some more senior Intel kernel developer's signed-off-by
 lines on this code before I will ever consider accepting it for the
 kernel.  Please do not resend this code until that happens.
 
 greg k-h

We apologize for wasting everyone's time and will certainly learn from this.
We won't resubmit the driver until a senior kernel developer has signed off on 
it.

Sincerely,
Sean O. Stalley
Stephanie S. Wallick
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/