Re: [PATCH 1/4] PCI MSI: Store the number of messages in the msi_desc

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7435 bytes --]

On Sat, 2008-07-05 at 09:34 -0400, Matthew Wilcox wrote:
> This first part simply changes the msi_attrib data structure to store
> how many vectors have been allocated.  In order to do this, I shrink the
> 'type' from 5 bits to 2 and rename it to _type to catch any unsuspecting
> users.

Please don't, it significantly uglifies the code IMHO. Just add a new
field for the size, I'd rather call it qsize to match the register.

If you're worried about bloating msi_desc, there's several fields in
there that are per-device not per-desc, so we could do another patch to
move them into pci_dev or something hanging off it, eg.
pci_dev->msi_info rather than storing them in every desc.

cheers


> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 8c61304..92992a8 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -106,11 +106,11 @@ static void msix_flush_writes(unsigned int irq)
>  
>  	entry = get_irq_msi(irq);
>  	BUG_ON(!entry || !entry->dev);
> -	switch (entry->msi_attrib.type) {
> -	case PCI_CAP_ID_MSI:
> +	switch (entry->msi_attrib._type) {
> +	case MSI_ATTRIB:
>  		/* nothing to do */
>  		break;
> -	case PCI_CAP_ID_MSIX:
> +	case MSIX_ATTRIB:
>  	{
>  		int offset = entry->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
>  			PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
> @@ -129,8 +129,8 @@ static void msi_set_mask_bits(unsigned int irq, u32 mask, u32 flag)
>  
>  	entry = get_irq_msi(irq);
>  	BUG_ON(!entry || !entry->dev);
> -	switch (entry->msi_attrib.type) {
> -	case PCI_CAP_ID_MSI:
> +	switch (entry->msi_attrib._type) {
> +	case MSI_ATTRIB:
>  		if (entry->msi_attrib.maskbit) {
>  			int pos;
>  			u32 mask_bits;
> @@ -144,7 +144,7 @@ static void msi_set_mask_bits(unsigned int irq, u32 mask, u32 flag)
>  			msi_set_enable(entry->dev, !flag);
>  		}
>  		break;
> -	case PCI_CAP_ID_MSIX:
> +	case MSIX_ATTRIB:
>  	{
>  		int offset = entry->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
>  			PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
> @@ -162,8 +162,8 @@ static void msi_set_mask_bits(unsigned int irq, u32 mask, u32 flag)
>  void read_msi_msg(unsigned int irq, struct msi_msg *msg)
>  {
>  	struct msi_desc *entry = get_irq_msi(irq);
> -	switch(entry->msi_attrib.type) {
> -	case PCI_CAP_ID_MSI:
> +	switch(entry->msi_attrib._type) {
> +	case MSI_ATTRIB:
>  	{
>  		struct pci_dev *dev = entry->dev;
>  		int pos = entry->msi_attrib.pos;
> @@ -182,7 +182,7 @@ void read_msi_msg(unsigned int irq, struct msi_msg *msg)
>  		msg->data = data;
>  		break;
>  	}
> -	case PCI_CAP_ID_MSIX:
> +	case MSIX_ATTRIB:
>  	{
>  		void __iomem *base;
>  		base = entry->mask_base +
> @@ -201,11 +201,17 @@ void read_msi_msg(unsigned int irq, struct msi_msg *msg)
>  void write_msi_msg(unsigned int irq, struct msi_msg *msg)
>  {
>  	struct msi_desc *entry = get_irq_msi(irq);
> -	switch (entry->msi_attrib.type) {
> -	case PCI_CAP_ID_MSI:
> +	switch (entry->msi_attrib._type) {
> +	case MSI_ATTRIB:
>  	{
>  		struct pci_dev *dev = entry->dev;
>  		int pos = entry->msi_attrib.pos;
> +		u16 msgctl;
> +
> +		pci_read_config_word(dev, msi_control_reg(pos), &msgctl);
> +		msgctl &= ~PCI_MSI_FLAGS_QSIZE;
> +		msgctl |= entry->msi_attrib.multiple << 4;
> +		pci_write_config_word(dev, msi_control_reg(pos), msgctl);
>  
>  		pci_write_config_dword(dev, msi_lower_address_reg(pos),
>  					msg->address_lo);
> @@ -220,7 +226,7 @@ void write_msi_msg(unsigned int irq, struct msi_msg *msg)
>  		}
>  		break;
>  	}
> -	case PCI_CAP_ID_MSIX:
> +	case MSIX_ATTRIB:
>  	{
>  		void __iomem *base;
>  		base = entry->mask_base +
> @@ -359,7 +365,7 @@ static int msi_capability_init(struct pci_dev *dev)
>  	if (!entry)
>  		return -ENOMEM;
>  
> -	entry->msi_attrib.type = PCI_CAP_ID_MSI;
> +	entry->msi_attrib._type = MSI_ATTRIB;
>  	entry->msi_attrib.is_64 = is_64bit_address(control);
>  	entry->msi_attrib.entry_nr = 0;
>  	entry->msi_attrib.maskbit = is_mask_bit_support(control);
> @@ -446,7 +452,7 @@ static int msix_capability_init(struct pci_dev *dev,
>  			break;
>  
>   		j = entries[i].entry;
> -		entry->msi_attrib.type = PCI_CAP_ID_MSIX;
> +		entry->msi_attrib._type = MSIX_ATTRIB;
>  		entry->msi_attrib.is_64 = 1;
>  		entry->msi_attrib.entry_nr = j;
>  		entry->msi_attrib.maskbit = 1;
> @@ -589,12 +595,13 @@ void pci_msi_shutdown(struct pci_dev* dev)
>  		u32 mask = entry->msi_attrib.maskbits_mask;
>  		msi_set_mask_bits(dev->irq, mask, ~mask);
>  	}
> -	if (!entry->dev || entry->msi_attrib.type != PCI_CAP_ID_MSI)
> +	if (!entry->dev || entry->msi_attrib._type != MSI_ATTRIB)
>  		return;
>  
>  	/* Restore dev->irq to its default pin-assertion irq */
>  	dev->irq = entry->msi_attrib.default_irq;
>  }
> +
>  void pci_disable_msi(struct pci_dev* dev)
>  {
>  	struct msi_desc *entry;
> @@ -605,7 +612,7 @@ void pci_disable_msi(struct pci_dev* dev)
>  	pci_msi_shutdown(dev);
>  
>  	entry = list_entry(dev->msi_list.next, struct msi_desc, list);
> -	if (!entry->dev || entry->msi_attrib.type != PCI_CAP_ID_MSI)
> +	if (!entry->dev || entry->msi_attrib._type != MSI_ATTRIB)
>  		return;
>  
>  	msi_free_irqs(dev);
> @@ -624,7 +631,7 @@ static int msi_free_irqs(struct pci_dev* dev)
>  	arch_teardown_msi_irqs(dev);
>  
>  	list_for_each_entry_safe(entry, tmp, &dev->msi_list, list) {
> -		if (entry->msi_attrib.type == PCI_CAP_ID_MSIX) {
> +		if (entry->msi_attrib._type == MSIX_ATTRIB) {
>  			writel(1, entry->mask_base + entry->msi_attrib.entry_nr
>  				  * PCI_MSIX_ENTRY_SIZE
>  				  + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
> diff --git a/drivers/pci/msi.h b/drivers/pci/msi.h
> index 3898f52..b72e0bd 100644
> --- a/drivers/pci/msi.h
> +++ b/drivers/pci/msi.h
> @@ -22,12 +22,8 @@
>  #define msi_disable(control)		control &= ~PCI_MSI_FLAGS_ENABLE
>  #define multi_msi_capable(control) \
>  	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
> -#define multi_msi_enable(control, num) \
> -	control |= (((num >> 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>  #define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
>  #define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
> -#define msi_enable(control, num) multi_msi_enable(control, num); \
> -	control |= PCI_MSI_FLAGS_ENABLE
>  
>  #define msix_table_offset_reg(base)	(base + 0x04)
>  #define msix_pba_offset_reg(base)	(base + 0x08)
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 8f29392..d322148 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -15,9 +15,13 @@ extern void unmask_msi_irq(unsigned int irq);
>  extern void read_msi_msg(unsigned int irq, struct msi_msg *msg);
>  extern void write_msi_msg(unsigned int irq, struct msi_msg *msg);
>  
> +#define MSI_ATTRIB	1
> +#define MSIX_ATTRIB	2
> +
>  struct msi_desc {
>  	struct {
> -		__u8	type	: 5; 	/* {0: unused, 5h:MSI, 11h:MSI-X} */
> +		__u8	_type	: 2; 	/* {0: unused, 1:MSI, 2:MSI-X} */
> +		__u8	multiple: 3;	/* log2 number of messages */
>  		__u8	maskbit	: 1; 	/* mask-pending bit supported ?   */
>  		__u8	masked	: 1;
>  		__u8	is_64	: 1;	/* Address size: 0=32bit 1=64bit  */
-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]