Message ID | 20120301071954.GA7152@elgon.mountain |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Dan Carpenter <dan.carpenter@oracle.com> Date: Thu, 1 Mar 2012 10:19:54 +0300 > "num_vfs" is a u32 but we only use the high 16 bits and the low 16bits > are left as zero. That isn't a problem for little endian systems but it > will break on big endian ones. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Dan, David. Just one quick comment... pci_enable_sriov's 2nd input is declared as type int and we were using u32 instead (for a non negative 16bit value). With a quick check I noticed that other pci_enable_sriov callers do something similar and may need to be taken care too: driver | type used ----------+-------------- mlx4 | int chelsio | unsigned int igb | unsigned int igbxe | unsigned int emulex | u32 vxge | u32 Another option would have been to make all drivers use int to match pci_enable_sriov (pci_enable_sriov->sriov_enable checks against negative values). (BTW, why is pci_enable_sriov prototype using int?) Thanks /Chris > -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Thursday, March 01, 2012 2:24 PM > To: dan.carpenter@oracle.com > Cc: Roopa Prabhu (roprabhu); Christian Benvenuti (benve); Neel Patel > (neepatel); Nishank Trivedi (nistrive); netdev@vger.kernel.org; kernel- > janitors@vger.kernel.org > Subject: Re: [patch] enic: fix an endian bug in enic_probe() > > From: Dan Carpenter <dan.carpenter@oracle.com> > Date: Thu, 1 Mar 2012 10:19:54 +0300 > > > "num_vfs" is a u32 but we only use the high 16 bits and the low > 16bits > > are left as zero. That isn't a problem for little endian systems but > it > > will break on big endian ones. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 01, 2012 at 07:23:45PM -0600, Christian Benvenuti (benve) wrote: > Thanks Dan, David. > > Just one quick comment... > pci_enable_sriov's 2nd input is declared as type int and we > were using u32 instead (for a non negative 16bit value). > With a quick check I noticed that other pci_enable_sriov callers > do something similar and may need to be taken care too: > The problem was calling pci_read_config_word() not pci_enable_sriov(). I did consider passing a temporary variable to pci_read_config_word() and then storing that in num_vfs but it seemed more complicated. Looking at the other drivers now, I see that's how some of them do it. But it makes sense for them because they have the num_vfs which the user can configure and the number from the hardware which is the max that are supported. Either way would fix it, but I feel that my fix is not terribly ugly. regards, dan carpenter
diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h index 99998c6..cf1fb4b 100644 --- a/drivers/net/ethernet/cisco/enic/enic.h +++ b/drivers/net/ethernet/cisco/enic/enic.h @@ -94,7 +94,7 @@ struct enic { u32 rx_coalesce_usecs; u32 tx_coalesce_usecs; #ifdef CONFIG_PCI_IOV - u32 num_vfs; + u16 num_vfs; #endif struct enic_port_profile *pp; diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c index e27ec1d..9080ed6 100644 --- a/drivers/net/ethernet/cisco/enic/enic_main.c +++ b/drivers/net/ethernet/cisco/enic/enic_main.c @@ -2390,7 +2390,7 @@ static int __devinit enic_probe(struct pci_dev *pdev, pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV); if (pos) { pci_read_config_word(pdev, pos + PCI_SRIOV_TOTAL_VF, - (u16 *)&enic->num_vfs); + &enic->num_vfs); if (enic->num_vfs) { err = pci_enable_sriov(pdev, enic->num_vfs); if (err) {
"num_vfs" is a u32 but we only use the high 16 bits and the low 16bits are left as zero. That isn't a problem for little endian systems but it will break on big endian ones. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html