On Tue, Sep 08, 2020 at 06:04:27PM +0300, Leon Romanovsky wrote:
> On Tue, Sep 08, 2020 at 11:46:46AM -0300, Jason Gunthorpe wrote:
> > On Tue, Sep 08, 2020 at 05:42:05PM +0300, Leon Romanovsky wrote:
> > > On Tue, Sep 08, 2020 at 11:27:56AM -0300, Jason Gunthorpe wrote:
> > > > On Tue, Sep 08, 2020 at 05:26:51PM +0300, Leon Romanovsky wrote:
> > > > > On Tue, Sep 08, 2020 at 11:19:24AM -0300, Jason Gunthorpe wrote:
> > > > > > On Wed, Sep 02, 2020 at 10:45:03AM +0300, Leon Romanovsky wrote:
> > > > > > > From: Aharon Landau <aharonl@xxxxxxxxxxxx>
> > > > > > >
> > > > > > > According to the IB spec active_speed size should be u16 and not u8 as
> > > > > > > before. Changing it to allow further extensions in offered speeds.
> > > > > > >
> > > > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > > > > Signed-off-by: Aharon Landau <aharonl@xxxxxxxxxxxx>
> > > > > > > Reviewed-by: Michael Guralnik <michaelgur@xxxxxxxxxx>
> > > > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxx>
> > > > > > > drivers/infiniband/core/uverbs_std_types_device.c | 3 ++-
> > > > > > > drivers/infiniband/core/verbs.c | 2 +-
> > > > > > > drivers/infiniband/hw/bnxt_re/bnxt_re.h | 2 +-
> > > > > > > drivers/infiniband/hw/hfi1/verbs.c | 2 +-
> > > > > > > drivers/infiniband/hw/mlx5/main.c | 8 ++------
> > > > > > > drivers/infiniband/hw/ocrdma/ocrdma_verbs.c | 2 +-
> > > > > > > drivers/infiniband/hw/qedr/verbs.c | 2 +-
> > > > > > > drivers/infiniband/hw/qib/qib.h | 6 +++---
> > > > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h | 2 +-
> > > > > > > include/rdma/ib_verbs.h | 4 ++--
> > > > > > > 10 files changed, 15 insertions(+), 18 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/infiniband/core/uverbs_std_types_device.c b/drivers/infiniband/core/uverbs_std_types_device.c
> > > > > > > index 75df2094a010..7b03446b6936 100644
> > > > > > > +++ b/drivers/infiniband/core/uverbs_std_types_device.c
> > > > > > > @@ -165,7 +165,8 @@ void copy_port_attr_to_resp(struct ib_port_attr *attr,
> > > > > > > resp->subnet_timeout = attr->subnet_timeout;
> > > > > > > resp->init_type_reply = attr->init_type_reply;
> > > > > > > resp->active_width = attr->active_width;
> > > > > > > - resp->active_speed = attr->active_speed;
> > > > > > > + WARN_ON(attr->active_speed & ~0xFF);
> > > > > >
> > > > > > ?? This doesn't seem like a warn on situation..
> > > > >
> > > > > Why? We are returning u8 to the user, so need to catch overflow.
> > > >
> > > > We need to have actual backwards compat here, not just throw a warning
> > > > at the syscall boundary
> > >
> > > We don't have fallback and don't have speed that crosses u8 limit yet.
> > > This WARN_ON() is needed to ensure that we properly extend
> > > ib_port_speed.
> >
> > Until we have some compat story I don't want to just increase this
> > value, it clearly renders the device unusable, so why do it?
>
> Because IBTA port speed extension was already approved, it is just
> not published yet.
So there should be a compat story. It can't just WARN_ON here.
The u8 active_speed should saturate at the highest speed we can
represent in the u8, and this will have to be updated somehow to pass
the u16 version.
Jason
