RE: [char-misc-next 4/4] mei: bus: enable non-blocking RX
From: Winkler, Tomas
Date: Thu Nov 17 2016 - 12:31:20 EST
>
> On Wed, Nov 16, 2016 at 10:51:30PM +0200, Tomas Winkler wrote:
> > From: Alexander Usyskin <alexander.usyskin@xxxxxxxxx>
> >
> > Enable non-blocking receive for drivers on mei bus, this allows
> > checking for data availability by mei client drivers. This is most
> > effective for fixed address clients, that lacks flow control.
> >
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@xxxxxxxxx>
> > Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> > ---
> > drivers/misc/mei/bus-fixup.c | 4 ++--
> > drivers/misc/mei/bus.c | 19 ++++++++++++++++---
> > drivers/misc/mei/mei_dev.h | 7 ++++++-
> > drivers/nfc/mei_phy.c | 7 ++++---
> > drivers/watchdog/mei_wdt.c | 2 +-
> > include/linux/mei_cl_bus.h | 3 ++-
> > 6 files changed, 31 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/misc/mei/bus-fixup.c
> > b/drivers/misc/mei/bus-fixup.c index 7f2cef9011ae..18e05ca7584f 100644
> > --- a/drivers/misc/mei/bus-fixup.c
> > +++ b/drivers/misc/mei/bus-fixup.c
> > @@ -141,7 +141,7 @@ static int mei_osver(struct mei_cl_device *cldev)
> > if (ret < 0)
> > return ret;
> >
> > - ret = __mei_cl_recv(cldev->cl, buf, length);
> > + ret = __mei_cl_recv(cldev->cl, buf, length, 0);
> > if (ret < 0)
> > return ret;
> >
> > @@ -272,7 +272,7 @@ static int mei_nfc_if_version(struct mei_cl *cl,
> > return -ENOMEM;
> >
> > ret = 0;
> > - bytes_recv = __mei_cl_recv(cl, (u8 *)reply, if_version_length);
> > + bytes_recv = __mei_cl_recv(cl, (u8 *)reply, if_version_length, 0);
> > if (bytes_recv < if_version_length) {
> > dev_err(bus->dev, "Could not read IF version\n");
> > ret = -EIO;
> > diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c index
> > 2fd254ecde2f..45e937937675 100644
> > --- a/drivers/misc/mei/bus.c
> > +++ b/drivers/misc/mei/bus.c
> > @@ -98,15 +98,18 @@ ssize_t __mei_cl_send(struct mei_cl *cl, u8 *buf,
> size_t length,
> > * @cl: host client
> > * @buf: buffer to receive
> > * @length: buffer length
> > + * @mode: io mode
> > *
> > * Return: read size in bytes of < 0 on error
> > */
> > -ssize_t __mei_cl_recv(struct mei_cl *cl, u8 *buf, size_t length)
> > +ssize_t __mei_cl_recv(struct mei_cl *cl, u8 *buf, size_t length,
> > + unsigned int mode)
> > {
> > struct mei_device *bus;
> > struct mei_cl_cb *cb;
> > size_t r_length;
> > ssize_t rets;
> > + bool nonblock = !!(mode & MEI_CL_IO_RX_NONBLOCK);
> >
> > if (WARN_ON(!cl || !cl->dev))
> > return -ENODEV;
> > @@ -127,6 +130,11 @@ ssize_t __mei_cl_recv(struct mei_cl *cl, u8 *buf,
> size_t length)
> > if (rets && rets != -EBUSY)
> > goto out;
> >
> > + if (nonblock) {
> > + rets = -EAGAIN;
> > + goto out;
> > + }
> > +
> > /* wait on event only if there is no other waiter */
> > /* synchronized under device mutex */
> > if (!waitqueue_active(&cl->rx_wait)) { @@ -197,14 +205,19 @@
> > EXPORT_SYMBOL_GPL(mei_cldev_send);
> > * @cldev: me client device
> > * @buf: buffer to receive
> > * @length: buffer length
> > + * @flags: read io flags [O_NONBLOCK]
> > *
> > * Return: read size in bytes of < 0 on error
> > */
> > -ssize_t mei_cldev_recv(struct mei_cl_device *cldev, u8 *buf, size_t
> > length)
> > +ssize_t mei_cldev_recv(struct mei_cl_device *cldev, u8 *buf, size_t length,
> > + unsigned int flags)
> > {
> > struct mei_cl *cl = cldev->cl;
> > + unsigned int mode;
> > +
> > + mode = (flags & O_NONBLOCK) ? MEI_CL_IO_RX_NONBLOCK : 0;
> >
> > - return __mei_cl_recv(cl, buf, length);
> > + return __mei_cl_recv(cl, buf, length, mode);
> > }
> > EXPORT_SYMBOL_GPL(mei_cldev_recv);
> >
> > diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
> > index 82e69a00b7a1..f16bd1209848 100644
> > --- a/drivers/misc/mei/mei_dev.h
> > +++ b/drivers/misc/mei/mei_dev.h
> > @@ -115,10 +115,14 @@ enum mei_cb_file_ops {
> > *
> > * @MEI_CL_IO_TX_BLOCKING: send is blocking
> > * @MEI_CL_IO_TX_INTERNAL: internal communication between driver and
> > FW
> > + *
> > + * @MEI_CL_IO_RX_NONBLOCK: recv is non-blocking
> > */
> > enum mei_cl_io_mode {
> > MEI_CL_IO_TX_BLOCKING = BIT(0),
> > MEI_CL_IO_TX_INTERNAL = BIT(1),
> > +
> > + MEI_CL_IO_RX_NONBLOCK = BIT(2),
> > };
> >
> > /*
> > @@ -318,7 +322,8 @@ void mei_cl_bus_rescan_work(struct work_struct
> > *work); void mei_cl_bus_dev_fixup(struct mei_cl_device *dev);
> > ssize_t __mei_cl_send(struct mei_cl *cl, u8 *buf, size_t length,
> > unsigned int mode);
> > -ssize_t __mei_cl_recv(struct mei_cl *cl, u8 *buf, size_t length);
> > +ssize_t __mei_cl_recv(struct mei_cl *cl, u8 *buf, size_t length,
> > + unsigned int mode);
> > bool mei_cl_bus_rx_event(struct mei_cl *cl); bool
> > mei_cl_bus_notify_event(struct mei_cl *cl); void
> > mei_cl_bus_remove_devices(struct mei_device *bus); diff --git
> > a/drivers/nfc/mei_phy.c b/drivers/nfc/mei_phy.c index
> > 8a04c5e02999..0e8158c5f81b 100644
> > --- a/drivers/nfc/mei_phy.c
> > +++ b/drivers/nfc/mei_phy.c
> > @@ -132,7 +132,8 @@ static int mei_nfc_if_version(struct nfc_mei_phy
> *phy)
> > if (!reply)
> > return -ENOMEM;
> >
> > - bytes_recv = mei_cldev_recv(phy->cldev, (u8 *)reply,
> if_version_length);
> > + bytes_recv = mei_cldev_recv(phy->cldev, (u8 *)reply,
> > + if_version_length, 0);
> > if (bytes_recv < 0 || bytes_recv < if_version_length) {
> > pr_err("Could not read IF version\n");
> > r = -EIO;
> > @@ -193,7 +194,7 @@ static int mei_nfc_connect(struct nfc_mei_phy *phy)
> > }
> >
> > bytes_recv = mei_cldev_recv(phy->cldev, (u8 *)reply,
> > - connect_resp_length);
> > + connect_resp_length, 0);
> > if (bytes_recv < 0) {
> > r = bytes_recv;
> > pr_err("Could not read connect response %d\n", r); @@ -279,7
> +280,7
> > @@ static int mei_nfc_recv(struct nfc_mei_phy *phy, u8 *buf, size_t length)
> > struct mei_nfc_hdr *hdr;
> > int received_length;
> >
> > - received_length = mei_cldev_recv(phy->cldev, buf, length);
> > + received_length = mei_cldev_recv(phy->cldev, buf, length, 0);
> > if (received_length < 0)
> > return received_length;
> >
> > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> > index b29c6fde7473..3e29af534f8e 100644
> > --- a/drivers/watchdog/mei_wdt.c
> > +++ b/drivers/watchdog/mei_wdt.c
> > @@ -423,7 +423,7 @@ static void mei_wdt_rx(struct mei_cl_device *cldev)
> > const size_t res_len = sizeof(res);
> > int ret;
> >
> > - ret = mei_cldev_recv(wdt->cldev, (u8 *)&res, res_len);
> > + ret = mei_cldev_recv(wdt->cldev, (u8 *)&res, res_len, 0);
> > if (ret < 0) {
> > dev_err(&cldev->dev, "failure in recv %d\n", ret);
> > return;
> > diff --git a/include/linux/mei_cl_bus.h b/include/linux/mei_cl_bus.h
> > index 017f5232b3de..c3bb565f0745 100644
> > --- a/include/linux/mei_cl_bus.h
> > +++ b/include/linux/mei_cl_bus.h
> > @@ -86,7 +86,8 @@ void mei_cldev_driver_unregister(struct mei_cl_driver
> *cldrv);
> > mei_cldev_driver_unregister)
> >
> > ssize_t mei_cldev_send(struct mei_cl_device *cldev, u8 *buf, size_t
> > length); -ssize_t mei_cldev_recv(struct mei_cl_device *cldev, u8
> > *buf, size_t length);
> > +ssize_t mei_cldev_recv(struct mei_cl_device *cldev, u8 *buf, size_t length,
> > + unsigned int flags);
>
> Functions like this are horrid! You have to always go around and dig up what
> "flags" means, and if 1 means this or 0 means that or whatever.
I agree the flags are always mystery but In this case we tried to follow known API, basically it just pushes O_NOBLOCK.
Second we made it similar to its send counterpart which takes few flags
>
> It makes it so you can not read the code that calls this function at all and know
> what is going on.
True, but consistency in API and relatively compact API are also in trade off in usefulness of an API.
>
> Just make a new function mei_cldev_recv_async() and then call a local, static
> function, that does the work with the correct flag set. That way the developer
> always knows exactly what is going on.
We can do a wrapper, but _async() is not proper here maybe _nonblock(),
Thanks
Tomas