Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1523792 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [char-misc-next 4/4] mei: bus: enable non-blocking RX Tomas Winkler <tomas.winkler@intel.com> - 2016-11-16 21:00 +0100
    Re: [char-misc-next 4/4] mei: bus: enable non-blocking RX Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-17 18:10 +0100
      RE: [char-misc-next 4/4] mei: bus: enable non-blocking RX "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-11-18 20:40 +0100
        Re: [char-misc-next 4/4] mei: bus: enable non-blocking RX Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-19 09:50 +0100
          RE: [char-misc-next 4/4] mei: bus: enable non-blocking RX "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-11-19 11:50 +0100
    RE: [char-misc-next 4/4] mei: bus: enable non-blocking RX "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-11-17 18:40 +0100
    Re: [char-misc-next 4/4] mei: bus: enable non-blocking RX Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-17 19:10 +0100

#1523792 — [char-misc-next 4/4] mei: bus: enable non-blocking RX

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);
 
 int mei_cldev_register_rx_cb(struct mei_cl_device *cldev, mei_cldev_cb_t rx_cb);
 int mei_cldev_register_notif_cb(struct mei_cl_device *cldev,
-- 
2.7.4

[toc] | [next] | [standalone]

#1524484

On Thu, Nov 17, 2016 at 04:22:24PM +0000, Winkler, Tomas wrote:
> > 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(),

Yes, I just guessed at the name :)

[toc] | [prev] | [next] | [standalone]

#1525676

> 
> On Thu, Nov 17, 2016 at 04:22:24PM +0000, Winkler, Tomas wrote:
> > > 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(),
> 
> Yes, I just guessed at the name :)

Understood, any how I believe that we should keep patch as is, there is no one
API _nonblock() function in the whole kernel unlike _async(), 
nonblock  is always passed in as a flag or Boolean. 

Tomas

[toc] | [prev] | [next] | [standalone]

#1525877

On Fri, Nov 18, 2016 at 07:30:25PM +0000, Winkler, Tomas wrote:
> 
> > 
> > On Thu, Nov 17, 2016 at 04:22:24PM +0000, Winkler, Tomas wrote:
> > > > 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(),
> > 
> > Yes, I just guessed at the name :)
> 
> Understood, any how I believe that we should keep patch as is, there is no one
> API _nonblock() function in the whole kernel unlike _async(), 
> nonblock  is always passed in as a flag or Boolean. 

And I'll still argue that this is a horrible api and you can do better.
No need to duplicate the errors of our childhood :)

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1525968

> 
> On Fri, Nov 18, 2016 at 07:30:25PM +0000, Winkler, Tomas wrote:
> >
> > >
> > > On Thu, Nov 17, 2016 at 04:22:24PM +0000, Winkler, Tomas wrote:
> > > > > 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(),
> > >
> > > Yes, I just guessed at the name :)
> >
> > Understood, any how I believe that we should keep patch as is, there
> > is no one API _nonblock() function in the whole kernel unlike
> > _async(), nonblock  is always passed in as a flag or Boolean.
> 
> And I'll still argue that this is a horrible api and you can do better.
> No need to duplicate the errors of our childhood :)

Legacy is not always wrong it as is consistency, and the balance where to put change of behavior in arguments or function names is gentle one.
I'm not sure we want to end with Java function names, either.
bool doesShorterNameExistThatEquallyConvaysTheBehaviorOfTheMethod()

I'll respin the patch as it's not really worth the argument in this particular case,  but  I'm not convinced.

Thanks
Tomas



> 
> thanks,
> 
> greg k-h

[toc] | [prev] | [next] | [standalone]

#1524591

> 
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1524643

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web