| Message ID | 20180830140129.24878-1-yuval.shaia@oracle.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | RDMA/vmw_pvrdma: Update device when link is doing down | expand |
On 8/30/18 7:01 AM, Yuval Shaia wrote: > When Ethernet function goes up the driver updates the device. > Add support to device quiesce operation as a result of netdev down > event. > > Since this operation might not be supported by the device yet a warning > is printed and not error. > > While there fix the error message in the netdev up event. > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > --- > drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h | 1 + > drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 11 ++++++++++- > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > index 6fd5a8f4e2f6..fe966393991c 100644 > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > @@ -167,6 +167,7 @@ enum pvrdma_device_ctl { > PVRDMA_DEVICE_CTL_ACTIVATE, /* Activate device. */ > PVRDMA_DEVICE_CTL_UNQUIESCE, /* Unquiesce device. */ > PVRDMA_DEVICE_CTL_RESET, /* Reset device. */ > + PVRDMA_DEVICE_CTL_QUIESCE, /* Quiesce device. */ > }; > Thanks but we don't need the quiesce call since we handle the netdev down event transparently in the hypervisor. The unquiesce is an exception to be honest and we should be handling that as well transparently in the hypervisor. So we might look to remove that in the future. Sorry. Is this something specifically needed for QEMU? > enum pvrdma_intr_vector { > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c > index a5719899f49a..0183faa2f54d 100644 > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c > @@ -701,6 +701,15 @@ static void pvrdma_netdevice_event_handle(struct pvrdma_dev *dev, > switch (event) { > case NETDEV_REBOOT: > case NETDEV_DOWN: > + pvrdma_write_reg(dev, PVRDMA_REG_CTL, > + PVRDMA_DEVICE_CTL_QUIESCE); > + > + mb(); > + > + if (pvrdma_read_reg(dev, PVRDMA_REG_ERR)) > + dev_warn(&dev->pdev->dev, > + "failed to quiesce device during link down\n"); > + > pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ERR); > break; > case NETDEV_UP: > @@ -711,7 +720,7 @@ static void pvrdma_netdevice_event_handle(struct pvrdma_dev *dev, > > if (pvrdma_read_reg(dev, PVRDMA_REG_ERR)) > dev_err(&dev->pdev->dev, > - "failed to activate device during link up\n"); > + "failed to unquiesce device during link up\n"); s/unquiesce/resume is better. > else > pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ACTIVE); > break; >
On Thu, Aug 30, 2018 at 04:00:34PM -0700, Adit Ranadive wrote: > On 8/30/18 7:01 AM, Yuval Shaia wrote: > > When Ethernet function goes up the driver updates the device. > > Add support to device quiesce operation as a result of netdev down > > event. > > > > Since this operation might not be supported by the device yet a warning > > is printed and not error. > > > > While there fix the error message in the netdev up event. > > > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > > --- > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h | 1 + > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 11 ++++++++++- > > 2 files changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > > index 6fd5a8f4e2f6..fe966393991c 100644 > > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > > @@ -167,6 +167,7 @@ enum pvrdma_device_ctl { > > PVRDMA_DEVICE_CTL_ACTIVATE, /* Activate device. */ > > PVRDMA_DEVICE_CTL_UNQUIESCE, /* Unquiesce device. */ > > PVRDMA_DEVICE_CTL_RESET, /* Reset device. */ > > + PVRDMA_DEVICE_CTL_QUIESCE, /* Quiesce device. */ > > }; > > > > Thanks but we don't need the quiesce call since we handle the netdev down > event transparently in the hypervisor. The unquiesce is an exception to be > honest and we should be handling that as well transparently in the hypervisor. > So we might look to remove that in the future. Sorry. > > Is this something specifically needed for QEMU? No. It make sense to do the same in QEMU. Just that this thought crossed my mind that if one side is covered by driver then the other should too. Again, i agree - this can be handled by the device, not the driver. Thanks, Yval > > > enum pvrdma_intr_vector { > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c > > index a5719899f49a..0183faa2f54d 100644 > > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c > > @@ -701,6 +701,15 @@ static void pvrdma_netdevice_event_handle(struct pvrdma_dev *dev, > > switch (event) { > > case NETDEV_REBOOT: > > case NETDEV_DOWN: > > + pvrdma_write_reg(dev, PVRDMA_REG_CTL, > > + PVRDMA_DEVICE_CTL_QUIESCE); > > + > > + mb(); > > + > > + if (pvrdma_read_reg(dev, PVRDMA_REG_ERR)) > > + dev_warn(&dev->pdev->dev, > > + "failed to quiesce device during link down\n"); > > + > > pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ERR); > > break; > > case NETDEV_UP: > > @@ -711,7 +720,7 @@ static void pvrdma_netdevice_event_handle(struct pvrdma_dev *dev, > > > > if (pvrdma_read_reg(dev, PVRDMA_REG_ERR)) > > dev_err(&dev->pdev->dev, > > - "failed to activate device during link up\n"); > > + "failed to unquiesce device during link up\n"); > > s/unquiesce/resume is better. > > > else > > pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ACTIVE); > > break; > > >
On Thu, Aug 30, 2018 at 05:01:29PM +0300, Yuval Shaia wrote: > When Ethernet function goes up the driver updates the device. > Add support to device quiesce operation as a result of netdev down > event. > > Since this operation might not be supported by the device yet a warning > is printed and not error. > > While there fix the error message in the netdev up event. > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h | 1 + > drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 11 ++++++++++- > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > index 6fd5a8f4e2f6..fe966393991c 100644 > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > @@ -167,6 +167,7 @@ enum pvrdma_device_ctl { > PVRDMA_DEVICE_CTL_ACTIVATE, /* Activate device. */ > PVRDMA_DEVICE_CTL_UNQUIESCE, /* Unquiesce device. */ > PVRDMA_DEVICE_CTL_RESET, /* Reset device. */ > + PVRDMA_DEVICE_CTL_QUIESCE, /* Quiesce device. */ > }; > > enum pvrdma_intr_vector { > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c > index a5719899f49a..0183faa2f54d 100644 > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c > @@ -701,6 +701,15 @@ static void pvrdma_netdevice_event_handle(struct pvrdma_dev *dev, > switch (event) { > case NETDEV_REBOOT: > case NETDEV_DOWN: > + pvrdma_write_reg(dev, PVRDMA_REG_CTL, > + PVRDMA_DEVICE_CTL_QUIESCE); > + > + mb(); Don't add mb()'s without a huge comment exmapling what it is supposed to be doing.. kinda looks wrong really. Jason
On Sat, Sep 01, 2018 at 03:18:49PM -0600, Jason Gunthorpe wrote: > On Thu, Aug 30, 2018 at 05:01:29PM +0300, Yuval Shaia wrote: > > When Ethernet function goes up the driver updates the device. > > Add support to device quiesce operation as a result of netdev down > > event. > > > > Since this operation might not be supported by the device yet a warning > > is printed and not error. > > > > While there fix the error message in the netdev up event. > > > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h | 1 + > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 11 ++++++++++- > > 2 files changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > > index 6fd5a8f4e2f6..fe966393991c 100644 > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > > @@ -167,6 +167,7 @@ enum pvrdma_device_ctl { > > PVRDMA_DEVICE_CTL_ACTIVATE, /* Activate device. */ > > PVRDMA_DEVICE_CTL_UNQUIESCE, /* Unquiesce device. */ > > PVRDMA_DEVICE_CTL_RESET, /* Reset device. */ > > + PVRDMA_DEVICE_CTL_QUIESCE, /* Quiesce device. */ > > }; > > > > enum pvrdma_intr_vector { > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c > > index a5719899f49a..0183faa2f54d 100644 > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c > > @@ -701,6 +701,15 @@ static void pvrdma_netdevice_event_handle(struct pvrdma_dev *dev, > > switch (event) { > > case NETDEV_REBOOT: > > case NETDEV_DOWN: > > + pvrdma_write_reg(dev, PVRDMA_REG_CTL, > > + PVRDMA_DEVICE_CTL_QUIESCE); > > + > > + mb(); > > Don't add mb()'s without a huge comment exmapling what it is supposed > to be doing.. kinda looks wrong really. > > Jason Actually it is a copy-paste of the pattern used for NETDEV_UP. Since pvrdma_write_reg write to register it is necessity to make sure it is written before advertising this fact as "done". Anyway, i dropped this patch since looks like it is not needed. Thanks, Yuval
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h index 6fd5a8f4e2f6..fe966393991c 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h @@ -167,6 +167,7 @@ enum pvrdma_device_ctl { PVRDMA_DEVICE_CTL_ACTIVATE, /* Activate device. */ PVRDMA_DEVICE_CTL_UNQUIESCE, /* Unquiesce device. */ PVRDMA_DEVICE_CTL_RESET, /* Reset device. */ + PVRDMA_DEVICE_CTL_QUIESCE, /* Quiesce device. */ }; enum pvrdma_intr_vector { diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c index a5719899f49a..0183faa2f54d 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c @@ -701,6 +701,15 @@ static void pvrdma_netdevice_event_handle(struct pvrdma_dev *dev, switch (event) { case NETDEV_REBOOT: case NETDEV_DOWN: + pvrdma_write_reg(dev, PVRDMA_REG_CTL, + PVRDMA_DEVICE_CTL_QUIESCE); + + mb(); + + if (pvrdma_read_reg(dev, PVRDMA_REG_ERR)) + dev_warn(&dev->pdev->dev, + "failed to quiesce device during link down\n"); + pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ERR); break; case NETDEV_UP: @@ -711,7 +720,7 @@ static void pvrdma_netdevice_event_handle(struct pvrdma_dev *dev, if (pvrdma_read_reg(dev, PVRDMA_REG_ERR)) dev_err(&dev->pdev->dev, - "failed to activate device during link up\n"); + "failed to unquiesce device during link up\n"); else pvrdma_dispatch_event(dev, 1, IB_EVENT_PORT_ACTIVE); break;
When Ethernet function goes up the driver updates the device. Add support to device quiesce operation as a result of netdev down event. Since this operation might not be supported by the device yet a warning is printed and not error. While there fix the error message in the netdev up event. Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> --- drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h | 1 + drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c | 11 ++++++++++- 2 files changed, 11 insertions(+), 1 deletion(-)