| Message ID | 1420824103-24169-3-git-send-email-wsa@the-dreams.de |
|---|---|
| State | RFC |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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?
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?
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
> > + 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!
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 --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.
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(+)