diff mbox

[RFC,02/11] i2c: add quirk checks to core

Message ID 1420824103-24169-3-git-send-email-wsa@the-dreams.de (mailing list archive)
State New, archived
Headers show

Commit Message

Wolfram Sang Jan. 9, 2015, 5:21 p.m. UTC
Let the core do the checks if HW quirks prevent a transfer. Saves code
from drivers and adds consistency.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/i2c-core.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

Comments

Sergei Shtylyov Jan. 9, 2015, 7:35 p.m. UTC | #1
Hello.

On 01/09/2015 08:21 PM, Wolfram Sang wrote:

> Let the core do the checks if HW quirks prevent a transfer. Saves code
> from drivers and adds consistency.

> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
>   drivers/i2c/i2c-core.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 39d25a8cb1ad..7b10a19abf5b 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -2063,6 +2063,56 @@ module_exit(i2c_exit);
>    * ----------------------------------------------------
>    */
>
> +/* Check if val is exceeding the quirk IFF quirk is non 0 */
> +#define i2c_quirk_exceeded(val, quirk) ((quirk) && ((val) > (quirk)))
> +
> +static int i2c_quirk_error(struct i2c_adapter *adap, struct i2c_msg *msg, char *err_msg)
> +{
> +	dev_err(&adap->dev, "quirk: %s (addr 0x%04x, size %u)\n", err_msg, msg->addr, msg->len);
> +	return -EOPNOTSUPP;
> +}

    Always returning the same value doesn't make much sense. Are you trying to 
save space on the call sites?

[...]
> @@ -2080,6 +2130,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>   	unsigned long orig_jiffies;
>   	int ret, try;
>
> +	if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))

    So, you only check for non-zero result of this function? Perhaps it makes 
sense to return true/false instead?

> +		return -EOPNOTSUPP;
> +

WBR, Sergei
Wolfram Sang Jan. 9, 2015, 8:45 p.m. UTC | #2
On Fri, Jan 09, 2015 at 10:35:27PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 01/09/2015 08:21 PM, Wolfram Sang wrote:
> 
> >Let the core do the checks if HW quirks prevent a transfer. Saves code
> >from drivers and adds consistency.
> 
> >Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> >---
> >  drivers/i2c/i2c-core.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >
> >diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >index 39d25a8cb1ad..7b10a19abf5b 100644
> >--- a/drivers/i2c/i2c-core.c
> >+++ b/drivers/i2c/i2c-core.c
> >@@ -2063,6 +2063,56 @@ module_exit(i2c_exit);
> >   * ----------------------------------------------------
> >   */
> >
> >+/* Check if val is exceeding the quirk IFF quirk is non 0 */
> >+#define i2c_quirk_exceeded(val, quirk) ((quirk) && ((val) > (quirk)))
> >+
> >+static int i2c_quirk_error(struct i2c_adapter *adap, struct i2c_msg *msg, char *err_msg)
> >+{
> >+	dev_err(&adap->dev, "quirk: %s (addr 0x%04x, size %u)\n", err_msg, msg->addr, msg->len);
> >+	return -EOPNOTSUPP;
> >+}
> 
>    Always returning the same value doesn't make much sense. Are you trying
> to save space on the call sites?

Please elaborate. I think it does. If a quirk matches, we report that we
don't support this transfer.

> [...]
> >@@ -2080,6 +2130,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> >  	unsigned long orig_jiffies;
> >  	int ret, try;
> >
> >+	if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
> 
>    So, you only check for non-zero result of this function? Perhaps it makes
> sense to return true/false instead?

Could be done, but what would be the advantage? A lot of functions
return errno or 0.
Sergei Shtylyov Jan. 9, 2015, 9:05 p.m. UTC | #3
Hello.

On 01/09/2015 11:45 PM, Wolfram Sang wrote:

>>> Let the core do the checks if HW quirks prevent a transfer. Saves code
>> >from drivers and adds consistency.

>>> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
>>> ---
>>>   drivers/i2c/i2c-core.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 53 insertions(+)
>>>
>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>>> index 39d25a8cb1ad..7b10a19abf5b 100644
>>> --- a/drivers/i2c/i2c-core.c
>>> +++ b/drivers/i2c/i2c-core.c
>>> @@ -2063,6 +2063,56 @@ module_exit(i2c_exit);
>>>    * ----------------------------------------------------
>>>    */
>>>
>>> +/* Check if val is exceeding the quirk IFF quirk is non 0 */
>>> +#define i2c_quirk_exceeded(val, quirk) ((quirk) && ((val) > (quirk)))
>>> +
>>> +static int i2c_quirk_error(struct i2c_adapter *adap, struct i2c_msg *msg, char *err_msg)
>>> +{
>>> +	dev_err(&adap->dev, "quirk: %s (addr 0x%04x, size %u)\n", err_msg, msg->addr, msg->len);
>>> +	return -EOPNOTSUPP;
>>> +}

>>     Always returning the same value doesn't make much sense. Are you trying
>> to save space on the call sites?

> Please elaborate. I think it does. If a quirk matches, we report that we
> don't support this transfer.

    OK, but what's the point of having this function return *int* if it always 
returns the same value? AFAIU, you're trying to save the code space on the 
call sites of this function by not having *return* -EOPNOTSUPP there each time?

>> [...]
>>> @@ -2080,6 +2130,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>>>   	unsigned long orig_jiffies;
>>>   	int ret, try;
>>>
>>> +	if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))

>>     So, you only check for non-zero result of this function? Perhaps it makes
>> sense to return true/false instead?

> Could be done, but what would be the advantage? A lot of functions
> return errno or 0.

    It would have been OK if you were actually caring about the result, e.g. 
returning it from __i2c_transfer(). Since you don't, IMO it would make more 
sense to return true from i2c_check_for_quirks() (making it *bool*) iff it did 
find/apply a quirk.

WBR, Sergei
Ludovic Desroches Jan. 12, 2015, 9:58 a.m. UTC | #4
Hi Wolfram,

On Fri, Jan 09, 2015 at 06:21:32PM +0100, Wolfram Sang wrote:
> Let the core do the checks if HW quirks prevent a transfer. Saves code
> from drivers and adds consistency.
> 
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
>  drivers/i2c/i2c-core.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 39d25a8cb1ad..7b10a19abf5b 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -2063,6 +2063,56 @@ module_exit(i2c_exit);
>   * ----------------------------------------------------
>   */
>  
> +/* Check if val is exceeding the quirk IFF quirk is non 0 */
> +#define i2c_quirk_exceeded(val, quirk) ((quirk) && ((val) > (quirk)))
> +
> +static int i2c_quirk_error(struct i2c_adapter *adap, struct i2c_msg *msg, char *err_msg)
> +{
> +	dev_err(&adap->dev, "quirk: %s (addr 0x%04x, size %u)\n", err_msg, msg->addr, msg->len);
> +	return -EOPNOTSUPP;
> +}
> +
> +static int i2c_check_for_quirks(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct i2c_adapter_quirks *q = adap->quirks;
> +	u16 max_read = q->max_read_len, max_write = q->max_write_len;
> +	int max_num = q->max_num_msgs, i;
> +
> +	if (q->flags & I2C_ADAPTER_QUIRK_COMB_WRITE_THEN_READ)
> +		max_num = 2;
> +
> +	if (i2c_quirk_exceeded(num, max_num))
> +		return i2c_quirk_error(adap, &msgs[0], "too many messages");
> +
> +	if (num == 2 && q->flags & I2C_ADAPTER_QUIRK_COMB_WRITE_FIRST) {
> +		if (msgs[0].flags & I2C_M_RD)
> +			return i2c_quirk_error(adap, &msgs[0], "invalid first write msg");
> +
> +		max_write = q->max_comb_write_len;
> +	}
> +
> +	if (num == 2 && q->flags & I2C_ADAPTER_QUIRK_COMB_READ_SECOND) {
> +		if (!(msgs[1].flags & I2C_M_RD) || msgs[0].addr != msgs[1].addr)
> +			return i2c_quirk_error(adap, &msgs[1], "invalid second read msg");
> +
> +		max_read = q->max_comb_read_len;
> +	}
> +
> +	for (i = 0; i < num; i++) {
> +		u16 len = msgs[i].len;
> +
> +		if (msgs[i].flags & I2C_M_RD) {
> +			if (i2c_quirk_exceeded(len, max_read))
> +				return i2c_quirk_error(adap, &msgs[i], "msg too long");
> +		} else {
> +			if (i2c_quirk_exceeded(len, max_write))
> +				return i2c_quirk_error(adap, &msgs[i], "msg too long");
> +		}
> +	}
> +

I am not sure it will perfectly fit at91 quirks.

The hardware can handle two messages by using the internal address
feature. The internal address size is from one byte to three bytes. Then
the length of the first message is limited to three but we don't have
this constraint for the second one. If we have 'write then read' no problem
but if we have two write messages, the second one will cause a quirk
exceeded error.

Regards

Ludovic

> +	return 0;
> +}
> +
>  /**
>   * __i2c_transfer - unlocked flavor of i2c_transfer
>   * @adap: Handle to I2C bus
> @@ -2080,6 +2130,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	unsigned long orig_jiffies;
>  	int ret, try;
>  
> +	if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
> +		return -EOPNOTSUPP;
> +
>  	/* i2c_trace_msg gets enabled when tracepoint i2c_transfer gets
>  	 * enabled.  This is an efficient way of keeping the for-loop from
>  	 * being executed when not needed.
> -- 
> 2.1.3
>
Wolfram Sang Jan. 12, 2015, 10:13 a.m. UTC | #5
> I am not sure it will perfectly fit at91 quirks.

I think it does.

> The hardware can handle two messages by using the internal address
> feature. The internal address size is from one byte to three bytes. Then
> the length of the first message is limited to three but we don't have
> this constraint for the second one. If we have 'write then read' no problem
> but if we have two write messages, the second one will cause a quirk
> exceeded error.

Yeah, for this reason I seperated I2C_ADAPTER_QUIRK_COMB_WRITE_FIRST
out. The first message is checked against max_comb_write_len which is
set to 3 for your driver. The second is checked agains max_write_len
which is unset in your driver and thus can be of any length.

That should work, no?
Russell King - ARM Linux Jan. 12, 2015, 12:08 p.m. UTC | #6
On Fri, Jan 09, 2015 at 06:21:32PM +0100, Wolfram Sang wrote:
> +static int i2c_quirk_error(struct i2c_adapter *adap, struct i2c_msg *msg, char *err_msg)
> +{
> +	dev_err(&adap->dev, "quirk: %s (addr 0x%04x, size %u)\n", err_msg, msg->addr, msg->len);
> +	return -EOPNOTSUPP;
> +}

So, what happens if I open an I2C adapter, find a message which causes
i2c_quirk_error() to be called, and then spin repeatedly calling that...
Shouldn't there be some rate limiting to this?
Matthias Brugger Jan. 12, 2015, 1:15 p.m. UTC | #7
2015-01-09 18:21 GMT+01:00 Wolfram Sang <wsa@the-dreams.de>:
> Let the core do the checks if HW quirks prevent a transfer. Saves code
> from drivers and adds consistency.
>
> Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> ---
>  drivers/i2c/i2c-core.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 39d25a8cb1ad..7b10a19abf5b 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -2063,6 +2063,56 @@ module_exit(i2c_exit);
>   * ----------------------------------------------------
>   */
>
> +/* Check if val is exceeding the quirk IFF quirk is non 0 */
> +#define i2c_quirk_exceeded(val, quirk) ((quirk) && ((val) > (quirk)))
> +
> +static int i2c_quirk_error(struct i2c_adapter *adap, struct i2c_msg *msg, char *err_msg)
> +{
> +       dev_err(&adap->dev, "quirk: %s (addr 0x%04x, size %u)\n", err_msg, msg->addr, msg->len);
> +       return -EOPNOTSUPP;
> +}
> +
> +static int i2c_check_for_quirks(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +       struct i2c_adapter_quirks *q = adap->quirks;
> +       u16 max_read = q->max_read_len, max_write = q->max_write_len;
> +       int max_num = q->max_num_msgs, i;
> +
> +       if (q->flags & I2C_ADAPTER_QUIRK_COMB_WRITE_THEN_READ)
> +               max_num = 2;
> +
> +       if (i2c_quirk_exceeded(num, max_num))
> +               return i2c_quirk_error(adap, &msgs[0], "too many messages");
> +
> +       if (num == 2 && q->flags & I2C_ADAPTER_QUIRK_COMB_WRITE_FIRST) {
> +               if (msgs[0].flags & I2C_M_RD)
> +                       return i2c_quirk_error(adap, &msgs[0], "invalid first write msg");
> +
> +               max_write = q->max_comb_write_len;
> +       }
> +
> +       if (num == 2 && q->flags & I2C_ADAPTER_QUIRK_COMB_READ_SECOND) {
> +               if (!(msgs[1].flags & I2C_M_RD) || msgs[0].addr != msgs[1].addr)
> +                       return i2c_quirk_error(adap, &msgs[1], "invalid second read msg");
> +
> +               max_read = q->max_comb_read_len;
> +       }
> +
> +       for (i = 0; i < num; i++) {
> +               u16 len = msgs[i].len;
> +
> +               if (msgs[i].flags & I2C_M_RD) {
> +                       if (i2c_quirk_exceeded(len, max_read))
> +                               return i2c_quirk_error(adap, &msgs[i], "msg too long");
> +               } else {
> +                       if (i2c_quirk_exceeded(len, max_write))
> +                               return i2c_quirk_error(adap, &msgs[i], "msg too long");
> +               }

What about being more verbose in the error message, specifying if it
was a read or a write message that failed?

> +       }
> +
> +       return 0;
> +}
> +
>  /**
>   * __i2c_transfer - unlocked flavor of i2c_transfer
>   * @adap: Handle to I2C bus
> @@ -2080,6 +2130,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>         unsigned long orig_jiffies;
>         int ret, try;
>
> +       if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
> +               return -EOPNOTSUPP;
> +
>         /* i2c_trace_msg gets enabled when tracepoint i2c_transfer gets
>          * enabled.  This is an efficient way of keeping the for-loop from
>          * being executed when not needed.
> --
> 2.1.3
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Wolfram Sang Feb. 24, 2015, 2:16 p.m. UTC | #8
> > +               if (msgs[i].flags & I2C_M_RD) {
> > +                       if (i2c_quirk_exceeded(len, max_read))
> > +                               return i2c_quirk_error(adap, &msgs[i], "msg too long");
> > +               } else {
> > +                       if (i2c_quirk_exceeded(len, max_write))
> > +                               return i2c_quirk_error(adap, &msgs[i], "msg too long");
> > +               }
> 
> What about being more verbose in the error message, specifying if it
> was a read or a write message that failed?

Yes, done now. Thanks!
Wolfram Sang Feb. 24, 2015, 2:25 p.m. UTC | #9
On Mon, Jan 12, 2015 at 12:08:14PM +0000, Russell King - ARM Linux wrote:
> On Fri, Jan 09, 2015 at 06:21:32PM +0100, Wolfram Sang wrote:
> > +static int i2c_quirk_error(struct i2c_adapter *adap, struct i2c_msg *msg, char *err_msg)
> > +{
> > +	dev_err(&adap->dev, "quirk: %s (addr 0x%04x, size %u)\n", err_msg, msg->addr, msg->len);
> > +	return -EOPNOTSUPP;
> > +}
> 
> So, what happens if I open an I2C adapter, find a message which causes
> i2c_quirk_error() to be called, and then spin repeatedly calling that...
> Shouldn't there be some rate limiting to this?

Can be argued. Changed to dev_err_ratelimited(). Thanks!
diff mbox

Patch

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 39d25a8cb1ad..7b10a19abf5b 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2063,6 +2063,56 @@  module_exit(i2c_exit);
  * ----------------------------------------------------
  */
 
+/* Check if val is exceeding the quirk IFF quirk is non 0 */
+#define i2c_quirk_exceeded(val, quirk) ((quirk) && ((val) > (quirk)))
+
+static int i2c_quirk_error(struct i2c_adapter *adap, struct i2c_msg *msg, char *err_msg)
+{
+	dev_err(&adap->dev, "quirk: %s (addr 0x%04x, size %u)\n", err_msg, msg->addr, msg->len);
+	return -EOPNOTSUPP;
+}
+
+static int i2c_check_for_quirks(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	struct i2c_adapter_quirks *q = adap->quirks;
+	u16 max_read = q->max_read_len, max_write = q->max_write_len;
+	int max_num = q->max_num_msgs, i;
+
+	if (q->flags & I2C_ADAPTER_QUIRK_COMB_WRITE_THEN_READ)
+		max_num = 2;
+
+	if (i2c_quirk_exceeded(num, max_num))
+		return i2c_quirk_error(adap, &msgs[0], "too many messages");
+
+	if (num == 2 && q->flags & I2C_ADAPTER_QUIRK_COMB_WRITE_FIRST) {
+		if (msgs[0].flags & I2C_M_RD)
+			return i2c_quirk_error(adap, &msgs[0], "invalid first write msg");
+
+		max_write = q->max_comb_write_len;
+	}
+
+	if (num == 2 && q->flags & I2C_ADAPTER_QUIRK_COMB_READ_SECOND) {
+		if (!(msgs[1].flags & I2C_M_RD) || msgs[0].addr != msgs[1].addr)
+			return i2c_quirk_error(adap, &msgs[1], "invalid second read msg");
+
+		max_read = q->max_comb_read_len;
+	}
+
+	for (i = 0; i < num; i++) {
+		u16 len = msgs[i].len;
+
+		if (msgs[i].flags & I2C_M_RD) {
+			if (i2c_quirk_exceeded(len, max_read))
+				return i2c_quirk_error(adap, &msgs[i], "msg too long");
+		} else {
+			if (i2c_quirk_exceeded(len, max_write))
+				return i2c_quirk_error(adap, &msgs[i], "msg too long");
+		}
+	}
+
+	return 0;
+}
+
 /**
  * __i2c_transfer - unlocked flavor of i2c_transfer
  * @adap: Handle to I2C bus
@@ -2080,6 +2130,9 @@  int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	unsigned long orig_jiffies;
 	int ret, try;
 
+	if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
+		return -EOPNOTSUPP;
+
 	/* i2c_trace_msg gets enabled when tracepoint i2c_transfer gets
 	 * enabled.  This is an efficient way of keeping the for-loop from
 	 * being executed when not needed.