| Message ID | 1515566393-63888-8-git-send-email-oulijun@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
On Wed, Jan 10, 2018 at 02:39:53PM +0800, Lijun Ou wrote: > This patch assgin a guid(Global Unique identifer) > value to the hip08 device. > > Signed-off-by: Lijun Ou <oulijun@huawei.com> > Signed-off-by: Yixian Liu <liuyixian@huawei.com> > Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> > --- > V2: > - Use the correct way to update guid from Leon > Romanovsky's review. > > V1: > - The initial submit > --- > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c > index a1839a3..2af4948 100644 > --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c > +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c > @@ -34,6 +34,7 @@ > #include <linux/etherdevice.h> > #include <linux/interrupt.h> > #include <linux/kernel.h> > +#include <net/addrconf.h> > #include <rdma/ib_umem.h> > > #include "hnae3.h" > @@ -4679,6 +4680,9 @@ static int hns_roce_hw_v2_get_cfg(struct hns_roce_dev *hr_dev, > hr_dev->iboe.netdevs[0] = handle->rinfo.netdev; > hr_dev->iboe.phy_port[0] = 0; > > + addrconf_addr_eui48((u8 *)&hr_dev->ib_dev.node_guid, > + hr_dev->iboe.netdevs[0]->dev_addr); > + Are you sure that it is right thing to do? ib_dev.node_guid is __be64 and you cast it for u8. Maybe, it is right, but I don't have ability to test it now. Thanks > for (i = 0; i < HNS_ROCE_V2_MAX_IRQ_NUM; i++) > hr_dev->irq[i] = pci_irq_vector(handle->pdev, > i + handle->rinfo.base_vector); > -- > 1.9.1 >
在 2018/1/10 14:49, Leon Romanovsky 写道: > On Wed, Jan 10, 2018 at 02:39:53PM +0800, Lijun Ou wrote: >> This patch assgin a guid(Global Unique identifer) >> value to the hip08 device. >> >> Signed-off-by: Lijun Ou <oulijun@huawei.com> >> Signed-off-by: Yixian Liu <liuyixian@huawei.com> >> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> >> --- >> V2: >> - Use the correct way to update guid from Leon >> Romanovsky's review. >> >> V1: >> - The initial submit >> --- >> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c >> index a1839a3..2af4948 100644 >> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c >> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c >> @@ -34,6 +34,7 @@ >> #include <linux/etherdevice.h> >> #include <linux/interrupt.h> >> #include <linux/kernel.h> >> +#include <net/addrconf.h> >> #include <rdma/ib_umem.h> >> >> #include "hnae3.h" >> @@ -4679,6 +4680,9 @@ static int hns_roce_hw_v2_get_cfg(struct hns_roce_dev *hr_dev, >> hr_dev->iboe.netdevs[0] = handle->rinfo.netdev; >> hr_dev->iboe.phy_port[0] = 0; >> >> + addrconf_addr_eui48((u8 *)&hr_dev->ib_dev.node_guid, >> + hr_dev->iboe.netdevs[0]->dev_addr); >> + > > Are you sure that it is right thing to do? > > ib_dev.node_guid is __be64 and you cast it for u8. > > Maybe, it is right, but I don't have ability to test it now. > > Thanks Yes, it can be get rightly. I have test it again The test result as follows: root@(none)# cat /sys/class/infiniband/hns_0/node_guid 5816:89ff:fe28:a34b Besides, I confirm the orign definition for addrconf_addr_eui48, it defined as follows: static inline void addrconf_addr_eui48(u8 *eui, const char *const addr) { addrconf_addr_eui48_base(eui, addr); eui[0] ^= 2; } The first input parameter type is u8 * and the patch [v4] {net,IB}/{rxe,usnic}: Utilize generic mac to eui32 function have the same usage. Thanks Lijun Ou > >> for (i = 0; i < HNS_ROCE_V2_MAX_IRQ_NUM; i++) >> hr_dev->irq[i] = pci_irq_vector(handle->pdev, >> i + handle->rinfo.base_vector); >> -- >> 1.9.1 >> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 10, 2018 at 05:23:41PM +0800, oulijun wrote: > 在 2018/1/10 14:49, Leon Romanovsky 写道: > > On Wed, Jan 10, 2018 at 02:39:53PM +0800, Lijun Ou wrote: > >> This patch assgin a guid(Global Unique identifer) > >> value to the hip08 device. > >> > >> Signed-off-by: Lijun Ou <oulijun@huawei.com> > >> Signed-off-by: Yixian Liu <liuyixian@huawei.com> > >> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> > >> --- > >> V2: > >> - Use the correct way to update guid from Leon > >> Romanovsky's review. > >> > >> V1: > >> - The initial submit > >> --- > >> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c > >> index a1839a3..2af4948 100644 > >> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c > >> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c > >> @@ -34,6 +34,7 @@ > >> #include <linux/etherdevice.h> > >> #include <linux/interrupt.h> > >> #include <linux/kernel.h> > >> +#include <net/addrconf.h> > >> #include <rdma/ib_umem.h> > >> > >> #include "hnae3.h" > >> @@ -4679,6 +4680,9 @@ static int hns_roce_hw_v2_get_cfg(struct hns_roce_dev *hr_dev, > >> hr_dev->iboe.netdevs[0] = handle->rinfo.netdev; > >> hr_dev->iboe.phy_port[0] = 0; > >> > >> + addrconf_addr_eui48((u8 *)&hr_dev->ib_dev.node_guid, > >> + hr_dev->iboe.netdevs[0]->dev_addr); > >> + > > > > Are you sure that it is right thing to do? > > > > ib_dev.node_guid is __be64 and you cast it for u8. > > > > Maybe, it is right, but I don't have ability to test it now. > > > > Thanks > Yes, it can be get rightly. I have test it again > The test result as follows: > > root@(none)# cat /sys/class/infiniband/hns_0/node_guid > 5816:89ff:fe28:a34b > > Besides, I confirm the orign definition for addrconf_addr_eui48, it defined as follows: > static inline void addrconf_addr_eui48(u8 *eui, const char *const addr) > { > addrconf_addr_eui48_base(eui, addr); > eui[0] ^= 2; > } > > The first input parameter type is u8 * and the patch [v4] {net,IB}/{rxe,usnic}: Utilize generic mac to eui32 function > have the same usage. Indeed the first parameter is u8, but you are converting from __be64 to be u8. Thanks > > Thanks > Lijun Ou > > > > >> for (i = 0; i < HNS_ROCE_V2_MAX_IRQ_NUM; i++) > >> hr_dev->irq[i] = pci_irq_vector(handle->pdev, > >> i + handle->rinfo.base_vector); > >> -- > >> 1.9.1 > >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 10, 2018 at 12:29:02PM +0200, Leon Romanovsky wrote: > On Wed, Jan 10, 2018 at 05:23:41PM +0800, oulijun wrote: > > 在 2018/1/10 14:49, Leon Romanovsky 写道: > > > On Wed, Jan 10, 2018 at 02:39:53PM +0800, Lijun Ou wrote: > > >> This patch assgin a guid(Global Unique identifer) > > >> value to the hip08 device. > > >> > > >> Signed-off-by: Lijun Ou <oulijun@huawei.com> > > >> Signed-off-by: Yixian Liu <liuyixian@huawei.com> > > >> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> > > >> V2: > > >> - Use the correct way to update guid from Leon > > >> Romanovsky's review. > > >> > > >> V1: > > >> - The initial submit > > >> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++++ > > >> 1 file changed, 4 insertions(+) > > >> > > >> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c > > >> index a1839a3..2af4948 100644 > > >> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c > > >> @@ -34,6 +34,7 @@ > > >> #include <linux/etherdevice.h> > > >> #include <linux/interrupt.h> > > >> #include <linux/kernel.h> > > >> +#include <net/addrconf.h> > > >> #include <rdma/ib_umem.h> > > >> > > >> #include "hnae3.h" > > >> @@ -4679,6 +4680,9 @@ static int hns_roce_hw_v2_get_cfg(struct hns_roce_dev *hr_dev, > > >> hr_dev->iboe.netdevs[0] = handle->rinfo.netdev; > > >> hr_dev->iboe.phy_port[0] = 0; > > >> > > >> + addrconf_addr_eui48((u8 *)&hr_dev->ib_dev.node_guid, > > >> + hr_dev->iboe.netdevs[0]->dev_addr); > > >> + > > > > > > Are you sure that it is right thing to do? > > > > > > ib_dev.node_guid is __be64 and you cast it for u8. > > > > > > Maybe, it is right, but I don't have ability to test it now. > > > > > > Thanks > > Yes, it can be get rightly. I have test it again > > The test result as follows: > > > > root@(none)# cat /sys/class/infiniband/hns_0/node_guid > > 5816:89ff:fe28:a34b > > > > Besides, I confirm the orign definition for addrconf_addr_eui48, it defined as follows: > > static inline void addrconf_addr_eui48(u8 *eui, const char *const addr) > > { > > addrconf_addr_eui48_base(eui, addr); > > eui[0] ^= 2; > > } > > > > The first input parameter type is u8 * and the patch [v4] {net,IB}/{rxe,usnic}: Utilize generic mac to eui32 function > > have the same usage. > > Indeed the first parameter is u8, but you are converting from __be64 to be u8. addrconf_addr_eui48 accepts an array of 6 u8's in big endian byte order representing the MAC. If you use a be64 to hold the MAC then it must be unnaturally shifted << 16, eg htobe64(0x1234 << 16); Will work to set the mac to 00:00:00:00:12:34 So, it is really weird to store a mac in a __be64, but it can be done correctly... The *really* sketchy thing is pssing something called the node_guid to something accepting a eui48. The node_guid is an eui64, and needs translation before it can become an eui48, so *something* is wrong and broken here. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
在 2018/1/10 18:29, Leon Romanovsky 写道: > On Wed, Jan 10, 2018 at 05:23:41PM +0800, oulijun wrote: >> 在 2018/1/10 14:49, Leon Romanovsky 写道: >>> On Wed, Jan 10, 2018 at 02:39:53PM +0800, Lijun Ou wrote: >>>> This patch assgin a guid(Global Unique identifer) >>>> value to the hip08 device. >>>> >>>> Signed-off-by: Lijun Ou <oulijun@huawei.com> >>>> Signed-off-by: Yixian Liu <liuyixian@huawei.com> >>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> >>>> --- >>>> V2: >>>> - Use the correct way to update guid from Leon >>>> Romanovsky's review. >>>> >>>> V1: >>>> - The initial submit >>>> --- >>>> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c >>>> index a1839a3..2af4948 100644 >>>> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c >>>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c >>>> @@ -34,6 +34,7 @@ >>>> #include <linux/etherdevice.h> >>>> #include <linux/interrupt.h> >>>> #include <linux/kernel.h> >>>> +#include <net/addrconf.h> >>>> #include <rdma/ib_umem.h> >>>> >>>> #include "hnae3.h" >>>> @@ -4679,6 +4680,9 @@ static int hns_roce_hw_v2_get_cfg(struct hns_roce_dev *hr_dev, >>>> hr_dev->iboe.netdevs[0] = handle->rinfo.netdev; >>>> hr_dev->iboe.phy_port[0] = 0; >>>> >>>> + addrconf_addr_eui48((u8 *)&hr_dev->ib_dev.node_guid, >>>> + hr_dev->iboe.netdevs[0]->dev_addr); >>>> + >>> >>> Are you sure that it is right thing to do? >>> >>> ib_dev.node_guid is __be64 and you cast it for u8. >>> >>> Maybe, it is right, but I don't have ability to test it now. >>> >>> Thanks >> Yes, it can be get rightly. I have test it again >> The test result as follows: >> >> root@(none)# cat /sys/class/infiniband/hns_0/node_guid >> 5816:89ff:fe28:a34b >> >> Besides, I confirm the orign definition for addrconf_addr_eui48, it defined as follows: >> static inline void addrconf_addr_eui48(u8 *eui, const char *const addr) >> { >> addrconf_addr_eui48_base(eui, addr); >> eui[0] ^= 2; >> } >> >> The first input parameter type is u8 * and the patch [v4] {net,IB}/{rxe,usnic}: Utilize generic mac to eui32 function >> have the same usage. > > Indeed the first parameter is u8, but you are converting from __be64 to be u8. > > Thanks > Yes, the type of node_guid is defined by ib core. the function of addrconf_addr_eui48 is also defined by ib core. I could not modify them. Did I need to change u8 to unsigned char? thanks >> >> Thanks >> Lijun Ou >> >>> >>>> for (i = 0; i < HNS_ROCE_V2_MAX_IRQ_NUM; i++) >>>> hr_dev->irq[i] = pci_irq_vector(handle->pdev, >>>> i + handle->rinfo.base_vector); >>>> -- >>>> 1.9.1 >>>> >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 11, 2018 at 10:49:57AM +0800, oulijun wrote: > >>>> @@ -4679,6 +4680,9 @@ static int hns_roce_hw_v2_get_cfg(struct hns_roce_dev *hr_dev, > >>>> hr_dev->iboe.netdevs[0] = handle->rinfo.netdev; > >>>> hr_dev->iboe.phy_port[0] = 0; > >>>> > >>>> + addrconf_addr_eui48((u8 *)&hr_dev->ib_dev.node_guid, > >>>> + hr_dev->iboe.netdevs[0]->dev_addr); > >>>> + > >>> > >>> Are you sure that it is right thing to do? > >>> > >>> ib_dev.node_guid is __be64 and you cast it for u8. > >>> > >>> Maybe, it is right, but I don't have ability to test it now. > >>> > >>> Thanks > >> Yes, it can be get rightly. I have test it again > >> The test result as follows: > >> > >> root@(none)# cat /sys/class/infiniband/hns_0/node_guid > >> 5816:89ff:fe28:a34b > >> > >> Besides, I confirm the orign definition for addrconf_addr_eui48, it defined as follows: > >> static inline void addrconf_addr_eui48(u8 *eui, const char *const addr) > >> { > >> addrconf_addr_eui48_base(eui, addr); > >> eui[0] ^= 2; > >> } > >> > >> The first input parameter type is u8 * and the patch [v4] {net,IB}/{rxe,usnic}: Utilize generic mac to eui32 function > >> have the same usage. > > > > Indeed the first parameter is u8, but you are converting from __be64 to be u8. > > > > Thanks > > > Yes, the type of node_guid is defined by ib core. the function of addrconf_addr_eui48 is also > defined by ib core. I could not modify them. Did I need to change u8 to unsigned char? Actually, now that I look more closely, what you have is right. It is just a very strange and misleading name of a function 'addrconf_addr_eui48' which takes in an eui48 and writes out a IB GUID. Would have expected 'addrconf_ib_guid_eui48' or something.. Even more confusing because the commit message talks about eui32 and misses a signed-off-by? Baffling. .. and there are dragons here, because the IPv6 interface id (eg what the similar addrconf_ifid_eui48 is handling) uses an inverted U/L bit to the IEEE EUI-64. .. and I can't quite recall what the IBTA eventually settled on as the standard for IB - if it was EUI-64 or modified IPv6 EUI-64 .. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 10, 2018 at 02:39:53PM +0800, Lijun Ou wrote: > This patch assgin a guid(Global Unique identifer) > value to the hip08 device. > > Signed-off-by: Lijun Ou <oulijun@huawei.com> > Signed-off-by: Yixian Liu <liuyixian@huawei.com> > Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> > --- > V2: > - Use the correct way to update guid from Leon > Romanovsky's review. > > V1: > - The initial submit > --- > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++++ > 1 file changed, 4 insertions(+) > Thanks, Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c index a1839a3..2af4948 100644 --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c @@ -34,6 +34,7 @@ #include <linux/etherdevice.h> #include <linux/interrupt.h> #include <linux/kernel.h> +#include <net/addrconf.h> #include <rdma/ib_umem.h> #include "hnae3.h" @@ -4679,6 +4680,9 @@ static int hns_roce_hw_v2_get_cfg(struct hns_roce_dev *hr_dev, hr_dev->iboe.netdevs[0] = handle->rinfo.netdev; hr_dev->iboe.phy_port[0] = 0; + addrconf_addr_eui48((u8 *)&hr_dev->ib_dev.node_guid, + hr_dev->iboe.netdevs[0]->dev_addr); + for (i = 0; i < HNS_ROCE_V2_MAX_IRQ_NUM; i++) hr_dev->irq[i] = pci_irq_vector(handle->pdev, i + handle->rinfo.base_vector);