Re: [PATCH v4 02/10] PCI/IDE: Enumerate Selective Stream IDE capabilities

[<dan.j.williams@intel.com>]

Jonathan Cameron wrote:
[..]
> > diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
> > new file mode 100644
> > index 000000000000..e15937cdb2a4
> > --- /dev/null
> > +++ b/drivers/pci/ide.c
> > @@ -0,0 +1,93 @@
> 
> > +static int __sel_ide_offset(u16 ide_cap, u8 nr_link_ide, u8 stream_index,
> > +			    u8 nr_ide_mem)
> > +{
> > +	u32 offset;
> > +
> > +	offset = ide_cap + PCI_IDE_LINK_STREAM_0 +
> > +		 nr_link_ide * PCI_IDE_LINK_BLOCK_SIZE;
> > +
> > +	/*
> > +	 * Assume a constant number of address association resources per
> > +	 * stream index
> > +	 */
> > +	offset += stream_index * PCI_IDE_SEL_BLOCK_SIZE(nr_ide_mem);
> > +	return offset;
> 
> 	return offset + stream_index * PCI_IDE_SEL_BLOCK_SIZE(nr_ide_mem);
> 
> is perhaps a little bit neater?

Sure.

> 
> > +}
> 
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index a3a3e942dedf..ab4ebf0f8a46 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > +/* Integrity and Data Encryption Extended Capability */
> > +#define PCI_IDE_CAP			0x4
> 
> Spec uses two digits. Things are a bit inconsistent in this file but
> 0x04 looks like the most common syntax if hex.  Curiously some are
> not in hex.  Anyhow, I'd go with 0x04 etc for all register offsets
> unless Bjorn or someone else shouts otherwise!

While I'm here, might as well.

> > +#define  PCI_IDE_CAP_LINK		0x1  /* Link IDE Stream Supported */
> > +#define  PCI_IDE_CAP_SELECTIVE		0x2  /* Selective IDE Streams Supported */
> > +#define  PCI_IDE_CAP_FLOWTHROUGH	0x4  /* Flow-Through IDE Stream Supported */
> > +#define  PCI_IDE_CAP_PARTIAL_HEADER_ENC 0x8  /* Partial Header Encryption Supported */
> > +#define  PCI_IDE_CAP_AGGREGATION	0x10 /* Aggregation Supported */
> > +#define  PCI_IDE_CAP_PCRC		0x20 /* PCRC Supported */
> > +#define  PCI_IDE_CAP_IDE_KM		0x40 /* IDE_KM Protocol Supported */
> > +#define  PCI_IDE_CAP_SEL_CFG		0x80 /* Selective IDE for Config Request Support */
> > +#define  PCI_IDE_CAP_ALG_MASK		__GENMASK(12, 8) /* Supported Algorithms */
> > +#define  PCI_IDE_CAP_ALG_AES_GCM_256	0    /* AES-GCM 256 key size, 96b MAC */
> Looking at the rest of this file I think this should be.
> #define  PCI_IDE_CAP_ALG_MASK		__GENMASK(12, 8) /* Supported Algorithms */
> #define   PCI_IDE_CAP_ALG_AES_GCM_256	0    /* AES-GCM 256 key size, 96b MAC */
> 
> So indent one more space. Example being PCI_LPH_LOC_NONE

ok.

> 
> > +#define  PCI_IDE_CAP_LINK_TC_NUM_MASK	__GENMASK(15, 13) /* Link IDE TCs */
> > +#define  PCI_IDE_CAP_SEL_NUM_MASK	__GENMASK(23, 16)/* Supported Selective IDE Streams */
> 
> Space before comment missing?

Got it.

> 
> > +#define  PCI_IDE_CAP_TEE_LIMITED	0x1000000 /* TEE-Limited Stream Supported */
> > +#define PCI_IDE_CTL			0x8
> As above 0x08 more consistent with rest of the file.  Same for remaining cases.
> > +#define  PCI_IDE_CTL_FLOWTHROUGH_IDE	0x4  /* Flow-Through IDE Stream Enabled */
> > +
> > +#define PCI_IDE_LINK_STREAM_0		0xc  /* First Link Stream Register Block */
> > +#define  PCI_IDE_LINK_BLOCK_SIZE	8
> > +/* Link IDE Stream block, up to PCI_IDE_CAP_LINK_TC_NUM */
> > +#define PCI_IDE_LINK_CTL_0		   0x0               /* First Link Control Register Offset in block */
> 
> Event this I think should be 0x01 for consistency

You mean 0x00, right?

> 
> > +#define  PCI_IDE_LINK_CTL_EN		   0x1               /* Link IDE Stream Enable */
> > +#define  PCI_IDE_LINK_CTL_TX_AGGR_NPR_MASK __GENMASK(3, 2)   /* Tx Aggregation Mode NPR */
> > +#define  PCI_IDE_LINK_CTL_TX_AGGR_PR_MASK  __GENMASK(5, 4)   /* Tx Aggregation Mode PR */
> > +#define  PCI_IDE_LINK_CTL_TX_AGGR_CPL_MASK __GENMASK(7, 6)   /* Tx Aggregation Mode CPL */
> > +#define  PCI_IDE_LINK_CTL_PCRC_EN	   0x100	     /* PCRC Enable */
> > +#define  PCI_IDE_LINK_CTL_PART_ENC_MASK	   __GENMASK(13, 10) /* Partial Header Encryption Mode */
> > +#define  PCI_IDE_LINK_CTL_ALG_MASK	   __GENMASK(18, 14) /* Selection from PCI_IDE_CAP_ALG */
> > +#define  PCI_IDE_LINK_CTL_TC_MASK	   __GENMASK(21, 19) /* Traffic Class */
> > +#define  PCI_IDE_LINK_CTL_ID_MASK	   __GENMASK(31, 24) /* Stream ID */
> > +#define PCI_IDE_LINK_STS_0		   0x4               /* First Link Status Register Offset in block */
> > +#define  PCI_IDE_LINK_STS_STATE		   __GENMASK(3, 0)   /* Link IDE Stream State */
> > +#define  PCI_IDE_LINK_STS_RECVD_INTEGRITY_CHECK	0x80000000   /* Received Integrity Check Fail Msg */
> Naming here is drawing on stuff not in the Status register description (in 6.2 anyway which is what I'm
> checking against).  That just calls this Received IDE Fail Message.
> The text else where calls it out 'Upon transition from Secure to Insecure for any reason, other than
> corresponding Link/Selective IDE Stream Enable bit is Cleared, for a given Stream, the Port must transmit an
> IDE Fail Message indicating the Stream ID to the Partner port'
> 
> To me the integrity check naming doesn't really cover that.
> 
> I did some minimal digging. Your text matches 6.0. 

Will update to:

#define  PCI_IDE_LINK_STS_IDE_FAIL         0x80000000        /* IDE fail message received */

...and same for selective.

[ snip the other occurences of 0-padding register offsets ]