[PATCH 0/2] serdev API extension

[PATCH 0/2] serdev API extension

[Andrey Smirnov]

Rob,

These patches are a continuation of discussion about adding
functionality to serdev API we had off-list.

Being added are a function to do a blocking write capable of
dispatching multiple calls to write_buf() and some very minimalistic
bus locking API.

Any feedback is appreciated.

Thanks,
Andrey Smirnov

Andrey Smirnov (2):
  serdev: Add serdev_device_write subroutine
  serdev: Add minimal bus locking API

 drivers/tty/serdev/core.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/serdev.h    | 34 ++++++++++++++++++++++++++++------
 2 files changed, 66 insertions(+), 6 deletions(-)

-- 
2.9.3

[PATCH 1/2] serdev: Add serdev_device_write subroutine

[Andrey Smirnov]

Add serdev_device_write() which is a blocking call allowing to transfer
arbitraty amount of data (potentially exceeding amount that
serdev_device_write_buf can process in a single call)

Cc: cphealy@gmail.com
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-serial@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/tty/serdev/core.c | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/serdev.h    | 23 +++++++++++++++++------
 2 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index f4c6c90..759e834 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -128,6 +128,41 @@ int serdev_device_write_buf(struct serdev_device *serdev,
 }
 EXPORT_SYMBOL_GPL(serdev_device_write_buf);
 
+int serdev_device_write(struct serdev_device *serdev,
+			const unsigned char *buf, size_t count)
+{
+	int ret = count;
+
+	if (serdev->ops->write_wakeup)
+		return -EINVAL;
+
+	mutex_lock(&serdev->write_lock);
+
+	for (;;) {
+		size_t chunk;
+
+		reinit_completion(&serdev->write_wakeup);
+
+		chunk = serdev_device_write_buf(serdev, buf, count);
+		if (chunk < 0) {
+			ret = chunk;
+			goto done;
+		}
+
+		buf   += chunk;
+		count -= chunk;
+
+		if (!count)
+			break;
+
+		wait_for_completion(&serdev->write_wakeup);
+	}
+done:
+	mutex_unlock(&serdev->write_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(serdev_device_write);
+
 void serdev_device_write_flush(struct serdev_device *serdev)
 {
 	struct serdev_controller *ctrl = serdev->ctrl;
@@ -232,6 +267,8 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl)
 	serdev->dev.parent = &ctrl->dev;
 	serdev->dev.bus = &serdev_bus_type;
 	serdev->dev.type = &serdev_device_type;
+	init_completion(&serdev->write_wakeup);
+	mutex_init(&serdev->write_lock);
 	return serdev;
 }
 EXPORT_SYMBOL_GPL(serdev_device_alloc);
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 5176cdc..8f7aa35 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -35,16 +35,19 @@ struct serdev_device_ops {
 
 /**
  * struct serdev_device - Basic representation of an serdev device
- * @dev:	Driver model representation of the device.
- * @nr:		Device number on serdev bus.
- * @ctrl:	serdev controller managing this device.
- * @ops:	Device operations.
+ * @dev:	 Driver model representation of the device.
+ * @nr:		 Device number on serdev bus.
+ * @ctrl:	 serdev controller managing this device.
+ * @ops:	 Device operations.
+ * @write_wakeup Completion used by serdev_device_write internally
  */
 struct serdev_device {
 	struct device dev;
 	int nr;
 	struct serdev_controller *ctrl;
 	const struct serdev_device_ops *ops;
+	struct completion write_wakeup;
+	struct mutex write_lock;
 };
 
 static inline struct serdev_device *to_serdev_device(struct device *d)
@@ -162,10 +165,13 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl
 {
 	struct serdev_device *serdev = ctrl->serdev;
 
-	if (!serdev || !serdev->ops->write_wakeup)
+	if (!serdev)
 		return;
 
-	serdev->ops->write_wakeup(serdev);
+	if (serdev->ops->write_wakeup)
+		serdev->ops->write_wakeup(serdev);
+	else
+		complete(&serdev->write_wakeup);
 }
 
 static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
@@ -187,6 +193,7 @@ void serdev_device_close(struct serdev_device *);
 unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
 void serdev_device_set_flow_control(struct serdev_device *, bool);
 int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_t);
+int serdev_device_write(struct serdev_device *serdev, const unsigned char *buf, size_t count);
 void serdev_device_write_flush(struct serdev_device *);
 int serdev_device_write_room(struct serdev_device *);
 
@@ -227,6 +234,10 @@ static inline int serdev_device_write_buf(struct serdev_device *sdev, const unsi
 {
 	return -ENODEV;
 }
+static inline int serdev_device_write(struct serdev_device *sdev, const unsigned char *buf, size_t count)
+{
+	return -ENODEV;
+}
 static inline void serdev_device_write_flush(struct serdev_device *sdev) {}
 static inline int serdev_device_write_room(struct serdev_device *sdev)
 {
-- 
2.9.3

[PATCH 2/2] serdev: Add minimal bus locking API

[Andrey Smirnov]

Add minimal bus locking API which is useful for serial devices that
implement request-reply protocol

Cc: cphealy@gmail.com
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-serial@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/tty/serdev/core.c |  1 +
 include/linux/serdev.h    | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 759e834..20ca231 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -269,6 +269,7 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl)
 	serdev->dev.type = &serdev_device_type;
 	init_completion(&serdev->write_wakeup);
 	mutex_init(&serdev->write_lock);
+	mutex_init(&serdev->bus_lock);
 	return serdev;
 }
 EXPORT_SYMBOL_GPL(serdev_device_alloc);
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 8f7aa35..6b73a79 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -48,6 +48,7 @@ struct serdev_device {
 	const struct serdev_device_ops *ops;
 	struct completion write_wakeup;
 	struct mutex write_lock;
+	struct mutex bus_lock;
 };
 
 static inline struct serdev_device *to_serdev_device(struct device *d)
@@ -55,6 +56,16 @@ static inline struct serdev_device *to_serdev_device(struct device *d)
 	return container_of(d, struct serdev_device, dev);
 }
 
+static inline void serdev_device_bus_lock(struct serdev_device *serdev)
+{
+	mutex_lock(&serdev->bus_lock);
+}
+
+static inline void serdev_device_bus_unlock(struct serdev_device *serdev)
+{
+	mutex_unlock(&serdev->bus_lock);
+}
+
 /**
  * struct serdev_device_driver - serdev slave device driver
  * @driver:	serdev device drivers should initialize name field of this
-- 
2.9.3

Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine

[Andy Shevchenko]

On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Add serdev_device_write() which is a blocking call allowing to transfer
> arbitraty amount of data (potentially exceeding amount that
> serdev_device_write_buf can process in a single call)

> +int serdev_device_write(struct serdev_device *serdev,
> +                       const unsigned char *buf, size_t count)
> +{

> +       int ret = count;

If count by some reason bigger than INT_MAX...

> +
> +       if (serdev->ops->write_wakeup)
> +               return -EINVAL;
> +
> +       mutex_lock(&serdev->write_lock);
> +
> +       for (;;) {
> +               size_t chunk;
> +
> +               reinit_completion(&serdev->write_wakeup);
> +
> +               chunk = serdev_device_write_buf(serdev, buf, count);


> +               if (chunk < 0) {

This will never happen. What kind of test did you try?

> +                       ret = chunk;
> +                       goto done;
> +               }


> +
> +               buf   += chunk;
> +               count -= chunk;
> +

> +               if (!count)

What is supposed to be returned? Initial count? Does it make any sense?

> +                       break;

Perhaps you need to refactor this function.

> +
> +               wait_for_completion(&serdev->write_wakeup);
> +       }

> +done:

It would be nice to have a suffix, like

done_unlock:


But I'm pretty sure if you refactor the code in a smart way you will
not need it.

> +       mutex_unlock(&serdev->write_lock);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_write);

>  /**
>   * struct serdev_device - Basic representation of an serdev device
> - * @dev:       Driver model representation of the device.
> - * @nr:                Device number on serdev bus.
> - * @ctrl:      serdev controller managing this device.
> - * @ops:       Device operations.
> + * @dev:        Driver model representation of the device.
> + * @nr:                 Device number on serdev bus.
> + * @ctrl:       serdev controller managing this device.
> + * @ops:        Device operations.

Does it make sense to shift? I would think of shorter field names instead.

> + * @write_wakeup Completion used by serdev_device_write internally

Colon is missed.

Another filed is missed.

>   */
>  struct serdev_device {
>         struct device dev;
>         int nr;
>         struct serdev_controller *ctrl;
>         const struct serdev_device_ops *ops;
> +       struct completion write_wakeup;
> +       struct mutex write_lock;
>  };
>
>  static inline struct serdev_device *to_serdev_device(struct device *d)
> @@ -162,10 +165,13 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl
>  {
>         struct serdev_device *serdev = ctrl->serdev;
>
> -       if (!serdev || !serdev->ops->write_wakeup)
> +       if (!serdev)
>                 return;
>
> -       serdev->ops->write_wakeup(serdev);
> +       if (serdev->ops->write_wakeup)
> +               serdev->ops->write_wakeup(serdev);
> +       else
> +               complete(&serdev->write_wakeup);

By the way does this changes the possible context of application
(atomic / non-atomic)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] serdev: Add minimal bus locking API

[Andy Shevchenko]

On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Add minimal bus locking API which is useful for serial devices that
> implement request-reply protocol

Can you put an example here?

I'm not sure mutex is needed at all.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine

[Rob Herring]

On Tue, Mar 14, 2017 at 8:48 AM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Add serdev_device_write() which is a blocking call allowing to transfer
> arbitraty amount of data (potentially exceeding amount that
> serdev_device_write_buf can process in a single call)
>
> Cc: cphealy@gmail.com
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-serial@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/tty/serdev/core.c | 37 +++++++++++++++++++++++++++++++++++++
>  include/linux/serdev.h    | 23 +++++++++++++++++------
>  2 files changed, 54 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index f4c6c90..759e834 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -128,6 +128,41 @@ int serdev_device_write_buf(struct serdev_device *serdev,
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_write_buf);
>
> +int serdev_device_write(struct serdev_device *serdev,
> +                       const unsigned char *buf, size_t count)

_write vs. _write_buf are not all that clear what the difference is.
but I don't have a better name.

Perhaps a timeout param is needed? This could never complete if CTS
remains deasserted.

> +{
> +       int ret = count;
> +
> +       if (serdev->ops->write_wakeup)
> +               return -EINVAL;
> +
> +       mutex_lock(&serdev->write_lock);
> +
> +       for (;;) {
> +               size_t chunk;
> +
> +               reinit_completion(&serdev->write_wakeup);

Perhaps write_comp instead as write_wakeup is already a function name.

> +
> +               chunk = serdev_device_write_buf(serdev, buf, count);
> +               if (chunk < 0) {
> +                       ret = chunk;
> +                       goto done;
> +               }
> +
> +               buf   += chunk;
> +               count -= chunk;
> +
> +               if (!count)
> +                       break;
> +
> +               wait_for_completion(&serdev->write_wakeup);
> +       }
> +done:
> +       mutex_unlock(&serdev->write_lock);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(serdev_device_write);
> +
>  void serdev_device_write_flush(struct serdev_device *serdev)
>  {
>         struct serdev_controller *ctrl = serdev->ctrl;
> @@ -232,6 +267,8 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl)
>         serdev->dev.parent = &ctrl->dev;
>         serdev->dev.bus = &serdev_bus_type;
>         serdev->dev.type = &serdev_device_type;
> +       init_completion(&serdev->write_wakeup);
> +       mutex_init(&serdev->write_lock);
>         return serdev;
>  }
>  EXPORT_SYMBOL_GPL(serdev_device_alloc);
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index 5176cdc..8f7aa35 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -35,16 +35,19 @@ struct serdev_device_ops {
>
>  /**
>   * struct serdev_device - Basic representation of an serdev device
> - * @dev:       Driver model representation of the device.
> - * @nr:                Device number on serdev bus.
> - * @ctrl:      serdev controller managing this device.
> - * @ops:       Device operations.
> + * @dev:        Driver model representation of the device.
> + * @nr:                 Device number on serdev bus.
> + * @ctrl:       serdev controller managing this device.
> + * @ops:        Device operations.
> + * @write_wakeup Completion used by serdev_device_write internally
>   */
>  struct serdev_device {
>         struct device dev;
>         int nr;
>         struct serdev_controller *ctrl;
>         const struct serdev_device_ops *ops;
> +       struct completion write_wakeup;
> +       struct mutex write_lock;
>  };
>
>  static inline struct serdev_device *to_serdev_device(struct device *d)
> @@ -162,10 +165,13 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl
>  {
>         struct serdev_device *serdev = ctrl->serdev;
>
> -       if (!serdev || !serdev->ops->write_wakeup)
> +       if (!serdev)
>                 return;
>
> -       serdev->ops->write_wakeup(serdev);
> +       if (serdev->ops->write_wakeup)
> +               serdev->ops->write_wakeup(serdev);
> +       else
> +               complete(&serdev->write_wakeup);
>  }
>
>  static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
> @@ -187,6 +193,7 @@ void serdev_device_close(struct serdev_device *);
>  unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
>  void serdev_device_set_flow_control(struct serdev_device *, bool);
>  int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_t);
> +int serdev_device_write(struct serdev_device *serdev, const unsigned char *buf, size_t count);

Drop the param names to be consistent with the rest of the declarations.

>  void serdev_device_write_flush(struct serdev_device *);
>  int serdev_device_write_room(struct serdev_device *);
>
> @@ -227,6 +234,10 @@ static inline int serdev_device_write_buf(struct serdev_device *sdev, const unsi
>  {
>         return -ENODEV;
>  }
> +static inline int serdev_device_write(struct serdev_device *sdev, const unsigned char *buf, size_t count)
> +{
> +       return -ENODEV;
> +}
>  static inline void serdev_device_write_flush(struct serdev_device *sdev) {}
>  static inline int serdev_device_write_room(struct serdev_device *sdev)
>  {
> --
> 2.9.3
>

Re: [PATCH 2/2] serdev: Add minimal bus locking API

[Rob Herring]

On Tue, Mar 14, 2017 at 8:48 AM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Add minimal bus locking API which is useful for serial devices that
> implement request-reply protocol

It's assumed that there's a single client, so I think the client
drivers should manage any locking they need. Maybe that changes if we
have multiple slave devices using RS-485.

Rob

Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine

[Andrey Smirnov]

On Wed, Mar 15, 2017 at 7:18 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Mar 14, 2017 at 8:48 AM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> Add serdev_device_write() which is a blocking call allowing to transfer
>> arbitraty amount of data (potentially exceeding amount that
>> serdev_device_write_buf can process in a single call)
>>
>> Cc: cphealy@gmail.com
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: linux-serial@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  drivers/tty/serdev/core.c | 37 +++++++++++++++++++++++++++++++++++++
>>  include/linux/serdev.h    | 23 +++++++++++++++++------
>>  2 files changed, 54 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>> index f4c6c90..759e834 100644
>> --- a/drivers/tty/serdev/core.c
>> +++ b/drivers/tty/serdev/core.c
>> @@ -128,6 +128,41 @@ int serdev_device_write_buf(struct serdev_device *serdev,
>>  }
>>  EXPORT_SYMBOL_GPL(serdev_device_write_buf);
>>
>> +int serdev_device_write(struct serdev_device *serdev,
>> +                       const unsigned char *buf, size_t count)
>
> _write vs. _write_buf are not all that clear what the difference is.
> but I don't have a better name.
>

serdev_device_send?

> Perhaps a timeout param is needed? This could never complete if CTS
> remains deasserted.

Yeah, I think it is a good idea, I'll add that in v2.

>
>> +{
>> +       int ret = count;
>> +
>> +       if (serdev->ops->write_wakeup)
>> +               return -EINVAL;
>> +
>> +       mutex_lock(&serdev->write_lock);
>> +
>> +       for (;;) {
>> +               size_t chunk;
>> +
>> +               reinit_completion(&serdev->write_wakeup);
>
> Perhaps write_comp instead as write_wakeup is already a function name.
>

OK, will do.

>> +
>> +               chunk = serdev_device_write_buf(serdev, buf, count);
>> +               if (chunk < 0) {
>> +                       ret = chunk;
>> +                       goto done;
>> +               }
>> +
>> +               buf   += chunk;
>> +               count -= chunk;
>> +
>> +               if (!count)
>> +                       break;
>> +
>> +               wait_for_completion(&serdev->write_wakeup);
>> +       }
>> +done:
>> +       mutex_unlock(&serdev->write_lock);
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(serdev_device_write);
>> +
>>  void serdev_device_write_flush(struct serdev_device *serdev)
>>  {
>>         struct serdev_controller *ctrl = serdev->ctrl;
>> @@ -232,6 +267,8 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl)
>>         serdev->dev.parent = &ctrl->dev;
>>         serdev->dev.bus = &serdev_bus_type;
>>         serdev->dev.type = &serdev_device_type;
>> +       init_completion(&serdev->write_wakeup);
>> +       mutex_init(&serdev->write_lock);
>>         return serdev;
>>  }
>>  EXPORT_SYMBOL_GPL(serdev_device_alloc);
>> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
>> index 5176cdc..8f7aa35 100644
>> --- a/include/linux/serdev.h
>> +++ b/include/linux/serdev.h
>> @@ -35,16 +35,19 @@ struct serdev_device_ops {
>>
>>  /**
>>   * struct serdev_device - Basic representation of an serdev device
>> - * @dev:       Driver model representation of the device.
>> - * @nr:                Device number on serdev bus.
>> - * @ctrl:      serdev controller managing this device.
>> - * @ops:       Device operations.
>> + * @dev:        Driver model representation of the device.
>> + * @nr:                 Device number on serdev bus.
>> + * @ctrl:       serdev controller managing this device.
>> + * @ops:        Device operations.
>> + * @write_wakeup Completion used by serdev_device_write internally
>>   */
>>  struct serdev_device {
>>         struct device dev;
>>         int nr;
>>         struct serdev_controller *ctrl;
>>         const struct serdev_device_ops *ops;
>> +       struct completion write_wakeup;
>> +       struct mutex write_lock;
>>  };
>>
>>  static inline struct serdev_device *to_serdev_device(struct device *d)
>> @@ -162,10 +165,13 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl
>>  {
>>         struct serdev_device *serdev = ctrl->serdev;
>>
>> -       if (!serdev || !serdev->ops->write_wakeup)
>> +       if (!serdev)
>>                 return;
>>
>> -       serdev->ops->write_wakeup(serdev);
>> +       if (serdev->ops->write_wakeup)
>> +               serdev->ops->write_wakeup(serdev);
>> +       else
>> +               complete(&serdev->write_wakeup);
>>  }
>>
>>  static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
>> @@ -187,6 +193,7 @@ void serdev_device_close(struct serdev_device *);
>>  unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
>>  void serdev_device_set_flow_control(struct serdev_device *, bool);
>>  int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_t);
>> +int serdev_device_write(struct serdev_device *serdev, const unsigned char *buf, size_t count);
>
> Drop the param names to be consistent with the rest of the declarations.

OK, will do.

>
>>  void serdev_device_write_flush(struct serdev_device *);
>>  int serdev_device_write_room(struct serdev_device *);
>>
>> @@ -227,6 +234,10 @@ static inline int serdev_device_write_buf(struct serdev_device *sdev, const unsi
>>  {
>>         return -ENODEV;
>>  }
>> +static inline int serdev_device_write(struct serdev_device *sdev, const unsigned char *buf, size_t count)
>> +{
>> +       return -ENODEV;
>> +}
>>  static inline void serdev_device_write_flush(struct serdev_device *sdev) {}
>>  static inline int serdev_device_write_room(struct serdev_device *sdev)
>>  {
>> --
>> 2.9.3
>>

Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine

[Andrey Smirnov]

On Tue, Mar 14, 2017 at 4:17 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> Add serdev_device_write() which is a blocking call allowing to transfer
>> arbitraty amount of data (potentially exceeding amount that
>> serdev_device_write_buf can process in a single call)
>
>> +int serdev_device_write(struct serdev_device *serdev,
>> +                       const unsigned char *buf, size_t count)
>> +{
>
>> +       int ret = count;
>
> If count by some reason bigger than INT_MAX...
>

True, I was merely copying type signature of serdev_device_write_buf,
which would have the same problem. I can add an appropriate check to
the code, but at the same time, I'd love to know why it wasn't a
concern in the latter function.

>> +
>> +       if (serdev->ops->write_wakeup)
>> +               return -EINVAL;
>> +
>> +       mutex_lock(&serdev->write_lock);
>> +
>> +       for (;;) {
>> +               size_t chunk;
>> +
>> +               reinit_completion(&serdev->write_wakeup);
>> +
>> +               chunk = serdev_device_write_buf(serdev, buf, count);
>
>
>> +               if (chunk < 0) {
>
> This will never happen. What kind of test did you try?
>

None of my code using that function ever hit that condition, OTOH, I
am not sure why I should ignore the API's signed return type, which I
assume is so in order to facilitate negative error returns. My
thinking is that even if in the current codebase is incapable of ever
returning negative result today, that doesn't mean it won't ever be,
and IMHO the chances of that are no different from the chances of
someone passing 'count' that is bigger than INT_MAX. Given that I'd
rather keep the check until the type signature of
serdev_device_write_buf changes.

>> +                       ret = chunk;
>> +                       goto done;
>> +               }
>
>
>> +
>> +               buf   += chunk;
>> +               count -= chunk;
>> +
>
>> +               if (!count)
>
> What is supposed to be returned? Initial count? Does it make any sense?
>

That part of this code is not very well thought out. I think returning
number of bytes successfully written would make more sense. I'll
change it to do that in v2, but I am open to suggestions.

>> +                       break;
>
> Perhaps you need to refactor this function.
>
>> +
>> +               wait_for_completion(&serdev->write_wakeup);
>> +       }
>
>> +done:
>
> It would be nice to have a suffix, like
>
> done_unlock:
>
>
> But I'm pretty sure if you refactor the code in a smart way you will
> not need it.
>

Yeah, I agree, it should be possible to make that loop more concise.
I'll give it a try in v2.

>> +       mutex_unlock(&serdev->write_lock);
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(serdev_device_write);
>
>>  /**
>>   * struct serdev_device - Basic representation of an serdev device
>> - * @dev:       Driver model representation of the device.
>> - * @nr:                Device number on serdev bus.
>> - * @ctrl:      serdev controller managing this device.
>> - * @ops:       Device operations.
>> + * @dev:        Driver model representation of the device.
>> + * @nr:                 Device number on serdev bus.
>> + * @ctrl:       serdev controller managing this device.
>> + * @ops:        Device operations.
>
> Does it make sense to shift? I would think of shorter field names instead.

I can see how that might make the diff look nice, but OTOH, I'd rather
not limit myself to only ~4 letters.

>
>> + * @write_wakeup Completion used by serdev_device_write internally
>
> Colon is missed.
>
> Another filed is missed.

Yep, thank you, will fix in v2.

>
>>   */
>>  struct serdev_device {
>>         struct device dev;
>>         int nr;
>>         struct serdev_controller *ctrl;
>>         const struct serdev_device_ops *ops;
>> +       struct completion write_wakeup;
>> +       struct mutex write_lock;
>>  };
>>
>>  static inline struct serdev_device *to_serdev_device(struct device *d)
>> @@ -162,10 +165,13 @@ static inline void serdev_controller_write_wakeup(struct serdev_controller *ctrl
>>  {
>>         struct serdev_device *serdev = ctrl->serdev;
>>
>> -       if (!serdev || !serdev->ops->write_wakeup)
>> +       if (!serdev)
>>                 return;
>>
>> -       serdev->ops->write_wakeup(serdev);
>> +       if (serdev->ops->write_wakeup)
>> +               serdev->ops->write_wakeup(serdev);
>> +       else
>> +               complete(&serdev->write_wakeup);
>
> By the way does this changes the possible context of application
> (atomic / non-atomic)?

It should be possible to call complete() in atomic context, so I don't
think it does.

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 2/2] serdev: Add minimal bus locking API

[Andrey Smirnov]

On Tue, Mar 14, 2017 at 4:20 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> Add minimal bus locking API which is useful for serial devices that
>> implement request-reply protocol
>
> Can you put an example here?
>
> I'm not sure mutex is needed at all.

My use case is a "supervisory" microcontroller connected to SoC via
UART various aspects of which are exposed via MFD. I saw this kind of
"device design pattern" a number of times in my career, so I thought
and abstraction to help dealing with cases like that might be useful.

However, since Rob mentioned that API's expectation is that any such
locking is driver's responsibility, I'll drop this patch.

Thanks,
Andrey Smirnov

Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine

[Andy Shevchenko]

On Thu, Mar 16, 2017 at 4:23 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> On Tue, Mar 14, 2017 at 4:17 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov
>> <andrew.smirnov@gmail.com> wrote:

>>> +int serdev_device_write(struct serdev_device *serdev,
>>> +                       const unsigned char *buf, size_t count)
>>> +{
>>
>>> +       int ret = count;
>>
>> If count by some reason bigger than INT_MAX...

> True, I was merely copying type signature of serdev_device_write_buf,
> which would have the same problem. I can add an appropriate check to
> the code, but at the same time, I'd love to know why it wasn't a
> concern in the latter function.

Perhaps we may survive without changing it, just document that count
should not be bigger than INT_MAX.
OTOH it's counter intuitive to have size_t type which is used as int.

>>> +       for (;;) {
>>> +               size_t chunk;
>>> +
>>> +               reinit_completion(&serdev->write_wakeup);
>>> +
>>> +               chunk = serdev_device_write_buf(serdev, buf, count);
>>
>>
>>> +               if (chunk < 0) {
>>
>> This will never happen. What kind of test did you try?

> None of my code using that function ever hit that condition, OTOH, I
> am not sure why I should ignore the API's signed return type, which I
> assume is so in order to facilitate negative error returns. My
> thinking is that even if in the current codebase is incapable of ever
> returning negative result today, that doesn't mean it won't ever be,
> and IMHO the chances of that are no different from the chances of
> someone passing 'count' that is bigger than INT_MAX. Given that I'd
> rather keep the check until the type signature of
> serdev_device_write_buf changes.

You missed the point

size_t is *unsigned* type!

>>> +               if (!count)
>>
>> What is supposed to be returned? Initial count? Does it make any sense?

> I'll change it to do that in v2, but I am open to suggestions.

Since function lacks of description (or I missed it?) I am out of
knowledge what you are trying to achieve here.

> I can see how that might make the diff look nice, but OTOH, I'd rather
> not limit myself to only ~4 letters.

Up to you and Rob.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine

[Andrey Smirnov]

On Thu, Mar 16, 2017 at 8:09 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 16, 2017 at 4:23 PM, Andrey Smirnov
> <andrew.smirnov@gmail.com> wrote:
>> On Tue, Mar 14, 2017 at 4:17 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov
>>> <andrew.smirnov@gmail.com> wrote:
>
>>>> +int serdev_device_write(struct serdev_device *serdev,
>>>> +                       const unsigned char *buf, size_t count)
>>>> +{
>>>
>>>> +       int ret = count;
>>>
>>> If count by some reason bigger than INT_MAX...
>
>> True, I was merely copying type signature of serdev_device_write_buf,
>> which would have the same problem. I can add an appropriate check to
>> the code, but at the same time, I'd love to know why it wasn't a
>> concern in the latter function.
>
> Perhaps we may survive without changing it, just document that count
> should not be bigger than INT_MAX.
> OTOH it's counter intuitive to have size_t type which is used as int.
>
>>>> +       for (;;) {
>>>> +               size_t chunk;
>>>> +
>>>> +               reinit_completion(&serdev->write_wakeup);
>>>> +
>>>> +               chunk = serdev_device_write_buf(serdev, buf, count);
>>>
>>>
>>>> +               if (chunk < 0) {
>>>
>>> This will never happen. What kind of test did you try?
>
>> None of my code using that function ever hit that condition, OTOH, I
>> am not sure why I should ignore the API's signed return type, which I
>> assume is so in order to facilitate negative error returns. My
>> thinking is that even if in the current codebase is incapable of ever
>> returning negative result today, that doesn't mean it won't ever be,
>> and IMHO the chances of that are no different from the chances of
>> someone passing 'count' that is bigger than INT_MAX. Given that I'd
>> rather keep the check until the type signature of
>> serdev_device_write_buf changes.
>
> You missed the point
>
> size_t is *unsigned* type!
>

Ah! I did miss the point! Yes, that's a bug for sure, will fix in v2.
Good catch, thank you!

>>>> +               if (!count)
>>>
>>> What is supposed to be returned? Initial count? Does it make any sense?
>
>> I'll change it to do that in v2, but I am open to suggestions.
>
> Since function lacks of description (or I missed it?) I am out of
> knowledge what you are trying to achieve here.
>

There's a description of what I had in mind for this function in
commit message. It does need to be documented in the code, I agree.

Thanks,
Andrey Smirnov

Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine

[Rob Herring]

On Thu, Mar 16, 2017 at 8:33 AM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> On Wed, Mar 15, 2017 at 7:18 AM, Rob Herring <robh@kernel.org> wrote:
>> On Tue, Mar 14, 2017 at 8:48 AM, Andrey Smirnov
>> <andrew.smirnov@gmail.com> wrote:
>>> Add serdev_device_write() which is a blocking call allowing to transfer
>>> arbitraty amount of data (potentially exceeding amount that
>>> serdev_device_write_buf can process in a single call)
>>>
>>> Cc: cphealy@gmail.com
>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>> Cc: linux-serial@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>>> ---
>>>  drivers/tty/serdev/core.c | 37 +++++++++++++++++++++++++++++++++++++
>>>  include/linux/serdev.h    | 23 +++++++++++++++++------
>>>  2 files changed, 54 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>>> index f4c6c90..759e834 100644
>>> --- a/drivers/tty/serdev/core.c
>>> +++ b/drivers/tty/serdev/core.c
>>> @@ -128,6 +128,41 @@ int serdev_device_write_buf(struct serdev_device *serdev,
>>>  }
>>>  EXPORT_SYMBOL_GPL(serdev_device_write_buf);
>>>
>>> +int serdev_device_write(struct serdev_device *serdev,
>>> +                       const unsigned char *buf, size_t count)
>>
>> _write vs. _write_buf are not all that clear what the difference is.
>> but I don't have a better name.
>>
>
> serdev_device_send?

send vs. write_buf? Still not that clear.

>> Perhaps a timeout param is needed? This could never complete if CTS
>> remains deasserted.
>
> Yeah, I think it is a good idea, I'll add that in v2.

Maybe just combine the 2 functions and a timeout of 0 follows the
original behavior of serdev_device_write_buf. I'm not sure if we could
handle write_wakeup in that case though.

Rob