Re: [PATCH 1/3] MTD: m25p80: fix write return value.
Hello, On 21 May 2015 at 01:45, Brian Norris wrote: > On Thu, Apr 30, 2015 at 03:33:47PM +0200, Michal Suchanek wrote: >> The 'retlen' points to a variable representing the number of data bytes >> written/read (see include/linux/mtd/mtd.h) by the current invocation of >> the function. This variable must be set, not incremented. >> >> v2: clearer commit message >> >> Signed-off-by: Michal Suchanek >> Acked-by: Marek Vasut >> --- >> drivers/mtd/devices/m25p80.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c >> index 7c8b169..0b2bc21 100644 >> --- a/drivers/mtd/devices/m25p80.c >> +++ b/drivers/mtd/devices/m25p80.c >> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, >> size_t len, >> >> spi_sync(spi, ); >> >> - *retlen += m.actual_length - cmd_sz; >> + *retlen = m.actual_length - cmd_sz; > > This is very wrong. It gets a little better once you add your next > patches (which are also not good) since those patches reinterpret the > usage of retlen, but this one definitely does *not* stand a lone. > > I'll admit the API is a little odd here, but the callers of this > function (see spi_nor_write()) actually depend on calling this multiple > times, with the value incrementing each time so we get a summary total. > You're now clobbering this value. > > I'm willing to accept patches to improve this API, if you think that > would help. Or to add comments that document this. Yes, the only user of the retlen value ignores it but passes it on so without the following patch this one makes the passed on value different from before. For m25p80 this would be fixed by truncating the write in the driver and setting actual_length appropriately rather than returning an error when the message is too long. It might possibly break other drivers, though. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] MTD: m25p80: fix write return value.
Hello, On 21 May 2015 at 01:45, Brian Norris computersforpe...@gmail.com wrote: On Thu, Apr 30, 2015 at 03:33:47PM +0200, Michal Suchanek wrote: The 'retlen' points to a variable representing the number of data bytes written/read (see include/linux/mtd/mtd.h) by the current invocation of the function. This variable must be set, not incremented. v2: clearer commit message Signed-off-by: Michal Suchanek hramr...@gmail.com Acked-by: Marek Vasut ma...@denx.de --- drivers/mtd/devices/m25p80.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index 7c8b169..0b2bc21 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len, spi_sync(spi, m); - *retlen += m.actual_length - cmd_sz; + *retlen = m.actual_length - cmd_sz; This is very wrong. It gets a little better once you add your next patches (which are also not good) since those patches reinterpret the usage of retlen, but this one definitely does *not* stand a lone. I'll admit the API is a little odd here, but the callers of this function (see spi_nor_write()) actually depend on calling this multiple times, with the value incrementing each time so we get a summary total. You're now clobbering this value. I'm willing to accept patches to improve this API, if you think that would help. Or to add comments that document this. Yes, the only user of the retlen value ignores it but passes it on so without the following patch this one makes the passed on value different from before. For m25p80 this would be fixed by truncating the write in the driver and setting actual_length appropriately rather than returning an error when the message is too long. It might possibly break other drivers, though. Thanks Michal -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] MTD: m25p80: fix write return value.
On Thu, Apr 30, 2015 at 03:33:47PM +0200, Michal Suchanek wrote: > The 'retlen' points to a variable representing the number of data bytes > written/read (see include/linux/mtd/mtd.h) by the current invocation of > the function. This variable must be set, not incremented. > > v2: clearer commit message > > Signed-off-by: Michal Suchanek > Acked-by: Marek Vasut > --- > drivers/mtd/devices/m25p80.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index 7c8b169..0b2bc21 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, > size_t len, > > spi_sync(spi, ); > > - *retlen += m.actual_length - cmd_sz; > + *retlen = m.actual_length - cmd_sz; This is very wrong. It gets a little better once you add your next patches (which are also not good) since those patches reinterpret the usage of retlen, but this one definitely does *not* stand a lone. I'll admit the API is a little odd here, but the callers of this function (see spi_nor_write()) actually depend on calling this multiple times, with the value incrementing each time so we get a summary total. You're now clobbering this value. I'm willing to accept patches to improve this API, if you think that would help. Or to add comments that document this. > } > > static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor) Also, I'm a little confused because you sent two different patch series very close to each other, and this patch is in both of them. Please don't do that. Either send a single patch series that contains all patches (and is versions v2, v3, etc., as you revise the whole thing) or send completely independent patch series. Don't include the same patch in different series. Anyway, please consider my comments, and when you have something better, please resend everything. I'm not going to take either series as-is. Thanks, Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] MTD: m25p80: fix write return value.
On Thu, Apr 30, 2015 at 03:33:47PM +0200, Michal Suchanek wrote: The 'retlen' points to a variable representing the number of data bytes written/read (see include/linux/mtd/mtd.h) by the current invocation of the function. This variable must be set, not incremented. v2: clearer commit message Signed-off-by: Michal Suchanek hramr...@gmail.com Acked-by: Marek Vasut ma...@denx.de --- drivers/mtd/devices/m25p80.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index 7c8b169..0b2bc21 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len, spi_sync(spi, m); - *retlen += m.actual_length - cmd_sz; + *retlen = m.actual_length - cmd_sz; This is very wrong. It gets a little better once you add your next patches (which are also not good) since those patches reinterpret the usage of retlen, but this one definitely does *not* stand a lone. I'll admit the API is a little odd here, but the callers of this function (see spi_nor_write()) actually depend on calling this multiple times, with the value incrementing each time so we get a summary total. You're now clobbering this value. I'm willing to accept patches to improve this API, if you think that would help. Or to add comments that document this. } static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor) Also, I'm a little confused because you sent two different patch series very close to each other, and this patch is in both of them. Please don't do that. Either send a single patch series that contains all patches (and is versions v2, v3, etc., as you revise the whole thing) or send completely independent patch series. Don't include the same patch in different series. Anyway, please consider my comments, and when you have something better, please resend everything. I'm not going to take either series as-is. Thanks, Brian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] MTD: m25p80: fix write return value.
On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote: > The 'retlen' points to a variable representing the number of data bytes > written/read (see include/linux/mtd/mtd.h) by the current invocation of > the function. This variable must be set, not incremented. > > v2: clearer commit message This V2 goes past the diffstat, so that when the patch is applied, it doesn't end up in the log. > Signed-off-by: Michal Suchanek > Acked-by: Marek Vasut > --- > drivers/mtd/devices/m25p80.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Aka. V changes go here. I don't think there's a need to repost tho :) > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index 7c8b169..0b2bc21 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t > to, size_t len, > > spi_sync(spi, ); > > - *retlen += m.actual_length - cmd_sz; > + *retlen = m.actual_length - cmd_sz; > } > > static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor) Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] MTD: m25p80: fix write return value.
On 30 April 2015 at 20:43, Marek Vasut wrote: > On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote: >> The size of written data was added to user supplied value rather than >> written at the provided address. > > You might want to work on the commit message a little, something like > the following, but feel free to reword as seen fit. > > The 'retlen' points to a variable representing the number of data bytes > written/read (see include/linux/mtd/mtd.h) by the current invocation of > the function. This variable must be set, not incremented. > > Otherwise, the patch is correct I believe: > > Acked-by: Marek Vasut > ok, I will send an updated version. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] MTD: m25p80: fix write return value.
On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote: > The size of written data was added to user supplied value rather than > written at the provided address. You might want to work on the commit message a little, something like the following, but feel free to reword as seen fit. The 'retlen' points to a variable representing the number of data bytes written/read (see include/linux/mtd/mtd.h) by the current invocation of the function. This variable must be set, not incremented. Otherwise, the patch is correct I believe: Acked-by: Marek Vasut > Signed-off-by: Michal Suchanek > --- > drivers/mtd/devices/m25p80.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index 7c8b169..0b2bc21 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t > to, size_t len, > > spi_sync(spi, ); > > - *retlen += m.actual_length - cmd_sz; > + *retlen = m.actual_length - cmd_sz; > } > > static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor) Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] MTD: m25p80: fix write return value.
On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote: The size of written data was added to user supplied value rather than written at the provided address. You might want to work on the commit message a little, something like the following, but feel free to reword as seen fit. The 'retlen' points to a variable representing the number of data bytes written/read (see include/linux/mtd/mtd.h) by the current invocation of the function. This variable must be set, not incremented. Otherwise, the patch is correct I believe: Acked-by: Marek Vasut ma...@denx.de Signed-off-by: Michal Suchanek hramr...@gmail.com --- drivers/mtd/devices/m25p80.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index 7c8b169..0b2bc21 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len, spi_sync(spi, m); - *retlen += m.actual_length - cmd_sz; + *retlen = m.actual_length - cmd_sz; } static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor) Best regards, Marek Vasut -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] MTD: m25p80: fix write return value.
On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote: The 'retlen' points to a variable representing the number of data bytes written/read (see include/linux/mtd/mtd.h) by the current invocation of the function. This variable must be set, not incremented. v2: clearer commit message This V2 goes past the diffstat, so that when the patch is applied, it doesn't end up in the log. Signed-off-by: Michal Suchanek hramr...@gmail.com Acked-by: Marek Vasut ma...@denx.de --- drivers/mtd/devices/m25p80.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Aka. Vn changes go here. I don't think there's a need to repost tho :) diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index 7c8b169..0b2bc21 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len, spi_sync(spi, m); - *retlen += m.actual_length - cmd_sz; + *retlen = m.actual_length - cmd_sz; } static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor) Best regards, Marek Vasut -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] MTD: m25p80: fix write return value.
On 30 April 2015 at 20:43, Marek Vasut ma...@denx.de wrote: On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote: The size of written data was added to user supplied value rather than written at the provided address. You might want to work on the commit message a little, something like the following, but feel free to reword as seen fit. The 'retlen' points to a variable representing the number of data bytes written/read (see include/linux/mtd/mtd.h) by the current invocation of the function. This variable must be set, not incremented. Otherwise, the patch is correct I believe: Acked-by: Marek Vasut ma...@denx.de ok, I will send an updated version. Thanks Michal -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
