Re: [char-misc-next 4/4] mei: bus: enable non-blocking RX

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Nov 16, 2016 at 10:51:30PM +0200, Tomas Winkler wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> 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@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  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.

It makes it so you can not read the code that calls this function at all
and know what is going on.

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.

thanks,

greg k-h