| Message ID | 20190319091009.28740-2-leon@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 6734b2973565e36659e97e12ab0d0faf1d9f3fbe |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Fixes for HNS driver | expand |
On 19-Mar-19 11:10, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@mellanox.com> > > port_pd is treated as le32 in declaration and read, fix assignment to be > in le32 too. This change fixes the following compilation warnings. > > drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: warning: incorrect type > in assignment (different base types) > drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: expected restricted __le32 [usertype] port_pd > drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: got restricted __be32 [usertype] > > Fixes: 9a4435375cd1 ("IB/hns: Add driver files for hns RoCE driver") > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > --- > drivers/infiniband/hw/hns/hns_roce_ah.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/hns/hns_roce_ah.c b/drivers/infiniband/hw/hns/hns_roce_ah.c > index b3c8c45ec1e3..64e0c69b69c5 100644 > --- a/drivers/infiniband/hw/hns/hns_roce_ah.c > +++ b/drivers/infiniband/hw/hns/hns_roce_ah.c > @@ -70,7 +70,7 @@ struct ib_ah *hns_roce_create_ah(struct ib_pd *ibpd, > HNS_ROCE_VLAN_SL_BIT_MASK) << > HNS_ROCE_VLAN_SL_SHIFT; > > - ah->av.port_pd = cpu_to_be32(to_hr_pd(ibpd)->pdn | > + ah->av.port_pd = cpu_to_le32(to_hr_pd(ibpd)->pdn | > (rdma_ah_get_port_num(ah_attr) << > HNS_ROCE_PORT_NUM_SHIFT)); > ah->av.gid_index = grh->sgid_index; > The subject makes it sound like this is a cosmetic change (fix variable alignment), I would consider rephrasing it. Reviewed-by: Gal Pressman <galpress@amazon.com>
On Tue, Mar 26, 2019 at 02:55:18PM +0200, Gal Pressman wrote: > On 19-Mar-19 11:10, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@mellanox.com> > > > > port_pd is treated as le32 in declaration and read, fix assignment to be > > in le32 too. This change fixes the following compilation warnings. > > > > drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: warning: incorrect type > > in assignment (different base types) > > drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: expected restricted __le32 [usertype] port_pd > > drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: got restricted __be32 [usertype] > > > > Fixes: 9a4435375cd1 ("IB/hns: Add driver files for hns RoCE driver") > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > --- > > drivers/infiniband/hw/hns/hns_roce_ah.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/hw/hns/hns_roce_ah.c b/drivers/infiniband/hw/hns/hns_roce_ah.c > > index b3c8c45ec1e3..64e0c69b69c5 100644 > > --- a/drivers/infiniband/hw/hns/hns_roce_ah.c > > +++ b/drivers/infiniband/hw/hns/hns_roce_ah.c > > @@ -70,7 +70,7 @@ struct ib_ah *hns_roce_create_ah(struct ib_pd *ibpd, > > HNS_ROCE_VLAN_SL_BIT_MASK) << > > HNS_ROCE_VLAN_SL_SHIFT; > > > > - ah->av.port_pd = cpu_to_be32(to_hr_pd(ibpd)->pdn | > > + ah->av.port_pd = cpu_to_le32(to_hr_pd(ibpd)->pdn | > > (rdma_ah_get_port_num(ah_attr) << > > HNS_ROCE_PORT_NUM_SHIFT)); > > ah->av.gid_index = grh->sgid_index; > > > > The subject makes it sound like this is a cosmetic change (fix variable > alignment), I would consider rephrasing it. > > Reviewed-by: Gal Pressman <galpress@amazon.com> Thanks Gal, I had an impression that "alignment" is common term to describe it. Any suggestions on how to rephrase it?
On 26-Mar-19 17:10, Leon Romanovsky wrote: > On Tue, Mar 26, 2019 at 02:55:18PM +0200, Gal Pressman wrote: >> On 19-Mar-19 11:10, Leon Romanovsky wrote: >>> From: Leon Romanovsky <leonro@mellanox.com> >>> >>> port_pd is treated as le32 in declaration and read, fix assignment to be >>> in le32 too. This change fixes the following compilation warnings. >>> >>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: warning: incorrect type >>> in assignment (different base types) >>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: expected restricted __le32 [usertype] port_pd >>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: got restricted __be32 [usertype] >>> >>> Fixes: 9a4435375cd1 ("IB/hns: Add driver files for hns RoCE driver") >>> Signed-off-by: Leon Romanovsky <leonro@mellanox.com> >>> --- >>> drivers/infiniband/hw/hns/hns_roce_ah.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/infiniband/hw/hns/hns_roce_ah.c b/drivers/infiniband/hw/hns/hns_roce_ah.c >>> index b3c8c45ec1e3..64e0c69b69c5 100644 >>> --- a/drivers/infiniband/hw/hns/hns_roce_ah.c >>> +++ b/drivers/infiniband/hw/hns/hns_roce_ah.c >>> @@ -70,7 +70,7 @@ struct ib_ah *hns_roce_create_ah(struct ib_pd *ibpd, >>> HNS_ROCE_VLAN_SL_BIT_MASK) << >>> HNS_ROCE_VLAN_SL_SHIFT; >>> >>> - ah->av.port_pd = cpu_to_be32(to_hr_pd(ibpd)->pdn | >>> + ah->av.port_pd = cpu_to_le32(to_hr_pd(ibpd)->pdn | >>> (rdma_ah_get_port_num(ah_attr) << >>> HNS_ROCE_PORT_NUM_SHIFT)); >>> ah->av.gid_index = grh->sgid_index; >>> >> >> The subject makes it sound like this is a cosmetic change (fix variable >> alignment), I would consider rephrasing it. >> >> Reviewed-by: Gal Pressman <galpress@amazon.com> > > Thanks Gal, > > I had an impression that "alignment" is common term to describe it. > Any suggestions on how to rephrase it? > You might be right, but personally this change was not what I expected after reading the subject. It's a nit, feel free to ignore my comment :). Maybe "Fix wrong endianness conversion of port_pd" is better?
On Tue, Mar 26, 2019 at 05:41:15PM +0200, Gal Pressman wrote: > On 26-Mar-19 17:10, Leon Romanovsky wrote: > > On Tue, Mar 26, 2019 at 02:55:18PM +0200, Gal Pressman wrote: > >> On 19-Mar-19 11:10, Leon Romanovsky wrote: > >>> From: Leon Romanovsky <leonro@mellanox.com> > >>> > >>> port_pd is treated as le32 in declaration and read, fix assignment to be > >>> in le32 too. This change fixes the following compilation warnings. > >>> > >>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: warning: incorrect type > >>> in assignment (different base types) > >>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: expected restricted __le32 [usertype] port_pd > >>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: got restricted __be32 [usertype] > >>> > >>> Fixes: 9a4435375cd1 ("IB/hns: Add driver files for hns RoCE driver") > >>> Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > >>> --- > >>> drivers/infiniband/hw/hns/hns_roce_ah.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/infiniband/hw/hns/hns_roce_ah.c b/drivers/infiniband/hw/hns/hns_roce_ah.c > >>> index b3c8c45ec1e3..64e0c69b69c5 100644 > >>> --- a/drivers/infiniband/hw/hns/hns_roce_ah.c > >>> +++ b/drivers/infiniband/hw/hns/hns_roce_ah.c > >>> @@ -70,7 +70,7 @@ struct ib_ah *hns_roce_create_ah(struct ib_pd *ibpd, > >>> HNS_ROCE_VLAN_SL_BIT_MASK) << > >>> HNS_ROCE_VLAN_SL_SHIFT; > >>> > >>> - ah->av.port_pd = cpu_to_be32(to_hr_pd(ibpd)->pdn | > >>> + ah->av.port_pd = cpu_to_le32(to_hr_pd(ibpd)->pdn | > >>> (rdma_ah_get_port_num(ah_attr) << > >>> HNS_ROCE_PORT_NUM_SHIFT)); > >>> ah->av.gid_index = grh->sgid_index; > >>> > >> > >> The subject makes it sound like this is a cosmetic change (fix variable > >> alignment), I would consider rephrasing it. > >> > >> Reviewed-by: Gal Pressman <galpress@amazon.com> > > > > Thanks Gal, > > > > I had an impression that "alignment" is common term to describe it. > > Any suggestions on how to rephrase it? > > > > You might be right, but personally this change was not what I expected after > reading the subject. It's a nit, feel free to ignore my comment :). > > Maybe "Fix wrong endianness conversion of port_pd" is better? I think so, Jason, do you want me to resend it? Thanks
On Tue, Mar 26, 2019 at 06:32:32PM +0200, Leon Romanovsky wrote: > On Tue, Mar 26, 2019 at 05:41:15PM +0200, Gal Pressman wrote: > > On 26-Mar-19 17:10, Leon Romanovsky wrote: > > > On Tue, Mar 26, 2019 at 02:55:18PM +0200, Gal Pressman wrote: > > >> On 19-Mar-19 11:10, Leon Romanovsky wrote: > > >>> From: Leon Romanovsky <leonro@mellanox.com> > > >>> > > >>> port_pd is treated as le32 in declaration and read, fix assignment to be > > >>> in le32 too. This change fixes the following compilation warnings. > > >>> > > >>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: warning: incorrect type > > >>> in assignment (different base types) > > >>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: expected restricted __le32 [usertype] port_pd > > >>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: got restricted __be32 [usertype] > > >>> > > >>> Fixes: 9a4435375cd1 ("IB/hns: Add driver files for hns RoCE driver") > > >>> Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > >>> drivers/infiniband/hw/hns/hns_roce_ah.c | 2 +- > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>> > > >>> diff --git a/drivers/infiniband/hw/hns/hns_roce_ah.c b/drivers/infiniband/hw/hns/hns_roce_ah.c > > >>> index b3c8c45ec1e3..64e0c69b69c5 100644 > > >>> +++ b/drivers/infiniband/hw/hns/hns_roce_ah.c > > >>> @@ -70,7 +70,7 @@ struct ib_ah *hns_roce_create_ah(struct ib_pd *ibpd, > > >>> HNS_ROCE_VLAN_SL_BIT_MASK) << > > >>> HNS_ROCE_VLAN_SL_SHIFT; > > >>> > > >>> - ah->av.port_pd = cpu_to_be32(to_hr_pd(ibpd)->pdn | > > >>> + ah->av.port_pd = cpu_to_le32(to_hr_pd(ibpd)->pdn | > > >>> (rdma_ah_get_port_num(ah_attr) << > > >>> HNS_ROCE_PORT_NUM_SHIFT)); > > >>> ah->av.gid_index = grh->sgid_index; > > >>> > > >> > > >> The subject makes it sound like this is a cosmetic change (fix variable > > >> alignment), I would consider rephrasing it. > > >> > > >> Reviewed-by: Gal Pressman <galpress@amazon.com> > > > > > > Thanks Gal, > > > > > > I had an impression that "alignment" is common term to describe it. > > > Any suggestions on how to rephrase it? > > > > > > > You might be right, but personally this change was not what I expected after > > reading the subject. It's a nit, feel free to ignore my comment :). > > > > Maybe "Fix wrong endianness conversion of port_pd" is better? Yes > I think so, > > Jason, do you want me to resend it? I want someone from hns to say that the changed swap is right.. I assume this code works as is ?? Jason
On 27-Mar-19 20:25, Jason Gunthorpe wrote: > On Tue, Mar 26, 2019 at 06:32:32PM +0200, Leon Romanovsky wrote: >> On Tue, Mar 26, 2019 at 05:41:15PM +0200, Gal Pressman wrote: >>> On 26-Mar-19 17:10, Leon Romanovsky wrote: >>>> On Tue, Mar 26, 2019 at 02:55:18PM +0200, Gal Pressman wrote: >>>>> On 19-Mar-19 11:10, Leon Romanovsky wrote: >>>>>> From: Leon Romanovsky <leonro@mellanox.com> >>>>>> >>>>>> port_pd is treated as le32 in declaration and read, fix assignment to be >>>>>> in le32 too. This change fixes the following compilation warnings. >>>>>> >>>>>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: warning: incorrect type >>>>>> in assignment (different base types) >>>>>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: expected restricted __le32 [usertype] port_pd >>>>>> drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: got restricted __be32 [usertype] >>>>>> >>>>>> Fixes: 9a4435375cd1 ("IB/hns: Add driver files for hns RoCE driver") >>>>>> Signed-off-by: Leon Romanovsky <leonro@mellanox.com> >>>>>> drivers/infiniband/hw/hns/hns_roce_ah.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_ah.c b/drivers/infiniband/hw/hns/hns_roce_ah.c >>>>>> index b3c8c45ec1e3..64e0c69b69c5 100644 >>>>>> +++ b/drivers/infiniband/hw/hns/hns_roce_ah.c >>>>>> @@ -70,7 +70,7 @@ struct ib_ah *hns_roce_create_ah(struct ib_pd *ibpd, >>>>>> HNS_ROCE_VLAN_SL_BIT_MASK) << >>>>>> HNS_ROCE_VLAN_SL_SHIFT; >>>>>> >>>>>> - ah->av.port_pd = cpu_to_be32(to_hr_pd(ibpd)->pdn | >>>>>> + ah->av.port_pd = cpu_to_le32(to_hr_pd(ibpd)->pdn | >>>>>> (rdma_ah_get_port_num(ah_attr) << >>>>>> HNS_ROCE_PORT_NUM_SHIFT)); >>>>>> ah->av.gid_index = grh->sgid_index; >>>>>> >>>>> >>>>> The subject makes it sound like this is a cosmetic change (fix variable >>>>> alignment), I would consider rephrasing it. >>>>> >>>>> Reviewed-by: Gal Pressman <galpress@amazon.com> >>>> >>>> Thanks Gal, >>>> >>>> I had an impression that "alignment" is common term to describe it. >>>> Any suggestions on how to rephrase it? >>>> >>> >>> You might be right, but personally this change was not what I expected after >>> reading the subject. It's a nit, feel free to ignore my comment :). >>> >>> Maybe "Fix wrong endianness conversion of port_pd" is better? > > Yes > >> I think so, >> >> Jason, do you want me to resend it? > > I want someone from hns to say that the changed swap is right.. I > assume this code works as is ?? I was wondering that as well, my guess is no one actually uses the port_num returned in query_ah flow. TBH, I don't really see a reason why port_pd should even have an endianness annotation.. But hns guys might have a better answer.
On Tue, Mar 19, 2019 at 11:10:08AM +0200, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@mellanox.com> > > port_pd is treated as le32 in declaration and read, fix assignment to be > in le32 too. This change fixes the following compilation warnings. > > drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: warning: incorrect type > in assignment (different base types) > drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: expected restricted __le32 [usertype] port_pd > drivers/infiniband/hw/hns/hns_roce_ah.c:67:24: got restricted __be32 [usertype] > > Fixes: 9a4435375cd1 ("IB/hns: Add driver files for hns RoCE driver") > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > Reviewed-by: Gal Pressman <galpress@amazon.com> > drivers/infiniband/hw/hns/hns_roce_ah.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) I just saw Lijun Ou reviewed this series, so applied to for-next Thanks, Jason
diff --git a/drivers/infiniband/hw/hns/hns_roce_ah.c b/drivers/infiniband/hw/hns/hns_roce_ah.c index b3c8c45ec1e3..64e0c69b69c5 100644 --- a/drivers/infiniband/hw/hns/hns_roce_ah.c +++ b/drivers/infiniband/hw/hns/hns_roce_ah.c @@ -70,7 +70,7 @@ struct ib_ah *hns_roce_create_ah(struct ib_pd *ibpd, HNS_ROCE_VLAN_SL_BIT_MASK) << HNS_ROCE_VLAN_SL_SHIFT; - ah->av.port_pd = cpu_to_be32(to_hr_pd(ibpd)->pdn | + ah->av.port_pd = cpu_to_le32(to_hr_pd(ibpd)->pdn | (rdma_ah_get_port_num(ah_attr) << HNS_ROCE_PORT_NUM_SHIFT)); ah->av.gid_index = grh->sgid_index;