PCIe r6.1, sec 6.30.1.1 defines a "DOE Discovery Version" field in
the DOE Discovery Request Data Object Contents (3rd DW) as:
15:8 DOE Discovery Version – must be 02h if the Capability Version in
the Data Object Exchange Extended Capability is 02h or greater.
Add support for the version on devices with the DOE v2 capability.
Signed-off-by: Alexey Kardashevskiy <[email protected]>
---
Changes:
v2:
* added the section number to the commit log
---
Does PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER need to be in pci-regs.h?
It is only going to be used by doe.c and yet a bunch of
PCI_DOE_DATA_OBJECT_DISC_REQ_3_xxx is already in doe.c.
I am asking as I have been told not to expose PCI_DOE_PROTOCOL_xxx guys
which is somehow different (?). Thanks,
---
include/uapi/linux/pci_regs.h | 1 +
drivers/pci/doe.c | 11 ++++++++---
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index a39193213ff2..b9c681f14181 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1144,6 +1144,7 @@
#define PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH 0x0003ffff
#define PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX 0x000000ff
+#define PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER 0x0000ff00
#define PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID 0x0000ffff
#define PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL 0x00ff0000
#define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX 0xff000000
diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index 61f0531d2b1d..f57def002175 100644
--- a/drivers/pci/doe.c
+++ b/drivers/pci/doe.c
@@ -381,11 +381,13 @@ static void pci_doe_task_complete(struct pci_doe_task *task)
complete(task->private);
}
-static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
+static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 capver, u8 *index, u16 *vid,
u8 *protocol)
{
+ u32 disver = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER,
+ (capver >= 2) ? 2 : 0);
u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
- *index);
+ *index) | disver;
__le32 request_pl_le = cpu_to_le32(request_pl);
__le32 response_pl_le;
u32 response_pl;
@@ -419,13 +421,16 @@ static int pci_doe_cache_protocols(struct pci_doe_mb *doe_mb)
{
u8 index = 0;
u8 xa_idx = 0;
+ u32 hdr = 0;
+
+ pci_read_config_dword(doe_mb->pdev, doe_mb->cap_offset, &hdr);
do {
int rc;
u16 vid;
u8 prot;
- rc = pci_doe_discovery(doe_mb, &index, &vid, &prot);
+ rc = pci_doe_discovery(doe_mb, PCI_EXT_CAP_VER(hdr), &index, &vid, &prot);
if (rc)
return rc;
--
2.41.0
[ add get_maintainer --git-blame folks to Cc ]
Alexey Kardashevskiy wrote:
> PCIe r6.1, sec 6.30.1.1 defines a "DOE Discovery Version" field in
> the DOE Discovery Request Data Object Contents (3rd DW) as:
>
> 15:8 DOE Discovery Version – must be 02h if the Capability Version in
> the Data Object Exchange Extended Capability is 02h or greater.
>
> Add support for the version on devices with the DOE v2 capability.
>
> Signed-off-by: Alexey Kardashevskiy <[email protected]>
Hey Alexey,
I notice that get_maintainer.pl by default only mentions Bjorn and
linux-pci to Cc on touches to drivers/pci/doe.c, but can you include all
the other folks that have worked on PCI DOE going forward? Same requiest
for SPDM and or anything else TSM related.
$ ./scripts/get_maintainer.pl --git-blame drivers/pci/doe.c
Bjorn Helgaas <[email protected]> (supporter:PCI SUBSYSTEM,modified commits:11/12=92%)
Dan Williams <[email protected]> (modified commits:15/12=100%)
Lukas Wunner <[email protected]> (modified commits:11/12=92%)
Jonathan Cameron <[email protected]> (modified commits:11/12=92%)
Ira Weiny <[email protected]> (modified commits:8/12=67%)
[email protected] (open list:PCI SUBSYSTEM)
[email protected] (open list)
Yes, in general "./scripts/get_maintainer.pl --git-blame", is too
chatty, but not for new code areas like this.
On Mon, Feb 26, 2024 at 02:31:14PM +1100, Alexey Kardashevskiy wrote:
> Does PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER need to be in pci-regs.h?
Yes that's fine.
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1144,6 +1144,7 @@
> #define PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH 0x0003ffff
>
> #define PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX 0x000000ff
> +#define PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER 0x0000ff00
> #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID 0x0000ffff
> #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL 0x00ff0000
> #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX 0xff000000
"DISCOVER" duplicates the preceding "DISC", maybe just
"PCI_DOE_DATA_OBJECT_DISC_REQ_3_VERSION" for simplicity?
> -static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
> +static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 capver, u8 *index, u16 *vid,
> u8 *protocol)
> {
> + u32 disver = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER,
> + (capver >= 2) ? 2 : 0);
> u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
> - *index);
> + *index) | disver;
Hm, why use a separate "disver" variable? This could be combined
into a single statement.
Subject should probably be "PCI/DOE: Support discovery version 2".
Otherwise LGTM.
Thanks,
Lukas
On 28/2/24 07:41, Lukas Wunner wrote:
> On Mon, Feb 26, 2024 at 02:31:14PM +1100, Alexey Kardashevskiy wrote:
>> Does PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER need to be in pci-regs.h?
>
> Yes that's fine.
>
>
>> --- a/include/uapi/linux/pci_regs.h
>> +++ b/include/uapi/linux/pci_regs.h
>> @@ -1144,6 +1144,7 @@
>> #define PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH 0x0003ffff
>>
>> #define PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX 0x000000ff
>> +#define PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER 0x0000ff00
>> #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID 0x0000ffff
>> #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL 0x00ff0000
>> #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX 0xff000000
>
> "DISCOVER" duplicates the preceding "DISC", maybe just
> "PCI_DOE_DATA_OBJECT_DISC_REQ_3_VERSION" for simplicity?
Well, mostly because the PCIe spec specifically says "discovery" in the
field description, not just "version", but ok, I'll drop it.
btw "DISC" is just confusing, it has nothing to do with discs. _PROTOCOL
is not even correct anymore, now, in PCIe r6.1 it is called "type",
lovely :) s/PCI_DOE_DATA_OBJECT_DISC_/PCI_DOE_DISCOVERY_/ (because
DO==DATA_OBJECT) imho would do better but may be some other day.
>
>
>> -static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
>> +static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 capver, u8 *index, u16 *vid,
>> u8 *protocol)
>> {
>> + u32 disver = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER,
>> + (capver >= 2) ? 2 : 0);
>> u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
>> - *index);
>> + *index) | disver;
>
> Hm, why use a separate "disver" variable? This could be combined
> into a single statement.
Less ugly since we want to keep it 80 chars long (do we, still?). Like
this looks meh:
{
u32 request_pl =
FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
*index) |
FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER,
(capver >= 2) ? 2 : 0);
__le32 request_pl_le = cpu_to_le32(request_pl);
If we did 100 chars, I could do:
{
u32 request_pl =
FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX, *index) |
FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_VER,
(capver >= 2) ? 2 : 0);
__le32 request_pl_le = cpu_to_le32(request_pl);
>
> Subject should probably be "PCI/DOE: Support discovery version 2".
> Otherwise LGTM.
Thanks! I'll repost soon.
>
> Thanks,
>
> Lukas
--
Alexey
On Tue, Mar 05, 2024 at 05:02:27PM +1100, Alexey Kardashevskiy wrote:
> On 28/2/24 07:41, Lukas Wunner wrote:
> > On Mon, Feb 26, 2024 at 02:31:14PM +1100, Alexey Kardashevskiy wrote:
> > > Does PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER need to be in pci-regs.h?
> >
> > Yes that's fine.
> >
> > > --- a/include/uapi/linux/pci_regs.h
> > > +++ b/include/uapi/linux/pci_regs.h
> > > @@ -1144,6 +1144,7 @@
> > > #define PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH 0x0003ffff
> > > #define PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX 0x000000ff
> > > +#define PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER 0x0000ff00
> > > #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID 0x0000ffff
> > > #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL 0x00ff0000
> > > #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX 0xff000000
> >
> > "DISCOVER" duplicates the preceding "DISC", maybe just
> > "PCI_DOE_DATA_OBJECT_DISC_REQ_3_VERSION" for simplicity?
>
> Well, mostly because the PCIe spec specifically says "discovery" in the
> field description, not just "version", but ok, I'll drop it.
>
> btw "DISC" is just confusing, it has nothing to do with discs. _PROTOCOL is
> not even correct anymore, now, in PCIe r6.1 it is called "type", lovely :)
> s/PCI_DOE_DATA_OBJECT_DISC_/PCI_DOE_DISCOVERY_/ (because DO==DATA_OBJECT)
> imho would do better but may be some other day.
Agreed, and there are only a couple uses so not hard to change,
although it is already in uapi/. Maybe nobody really uses it yet
though?
> Less ugly since we want to keep it 80 chars long (do we, still?). Like this
> looks meh:
>
> u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
> *index) |
> FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER,
> (capver >= 2) ? 2 : 0);
>
> __le32 request_pl_le = cpu_to_le32(request_pl);
>
> If we did 100 chars, I could do:
>
> u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
> *index) |
> FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_VER,
> (capver >= 2) ? 2 : 0);
> __le32 request_pl_le = cpu_to_le32(request_pl);
Personally I prefer 80 columns because all the rest of drivers/pci is
that, and it's a hassle when browsing to have to resize the window to
either accommodate wider lines or avoid wasting screen space when all
the lines are shorter.
But there are exceptions. Strings don't need to be broken because
grepping for error messages works better when they're continued. This
situation might be an exception, too. We don't need to slavishly
adhere to 80 if the result looks terrible.
Bjorn
On Tue, Mar 05, 2024 at 05:02:27PM +1100, Alexey Kardashevskiy wrote:
> On 28/2/24 07:41, Lukas Wunner wrote:
> > > + u32 disver = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER,
> > > + (capver >= 2) ? 2 : 0);
> > > u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
> > > - *index);
> > > + *index) | disver;
> >
> > Hm, why use a separate "disver" variable? This could be combined
> > into a single statement.
>
> Less ugly since we want to keep it 80 chars long
I don't quite follow, this stays exactly within 80 chars:
u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
- *index);
+ *index) |
+ FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_DISCOVER_VER,
+ (capver >= 2) ? 2 : 0);
And if you replace DISCOVER_VER with VERSION as I've suggested,
it becomes even shorter:
u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
- *index);
+ *index) |
+ FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_VERSION,
+ (capver >= 2) ? 2 : 0);
Thanks,
Lukas