Patchwork [03/14] mtd: define `is_ecc_error()' macros

login
register
mail settings
Submitter Brian Norris
Date Sept. 7, 2011, 8:13 p.m.
Message ID <1315426421-16243-4-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/113822/
State New
Headers show

Comments

Brian Norris - Sept. 7, 2011, 8:13 p.m.
These macros can be used instead of including -EUCLEAN and -EBADMSG all
over the place. They should help make code a little bit more readable.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/linux/mtd/mtd.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
Artem Bityutskiy - Sept. 11, 2011, 1:57 p.m.
On Wed, 2011-09-07 at 13:13 -0700, Brian Norris wrote:
> These macros can be used instead of including -EUCLEAN and -EBADMSG all
> over the place. They should help make code a little bit more readable.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  include/linux/mtd/mtd.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index ead70ed..1dd74f4 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -348,4 +348,9 @@ void *mtd_kmalloc_up_to(const struct mtd_info *mtd, size_t *size);
>  
>  void mtd_erase_callback(struct erase_info *instr);
>  
> +#define is_corrected_ecc_error(error) (error == -EUCLEAN)

This is not really an error...

> +#define is_uncorrected_ecc_error(error) (error == -EBADMSG)
> +#define is_ecc_error(error) ((is_corrected_ecc_error(error) || \
> +			      is_uncorrected_ecc_error(error)))

Could we please have static inline functions instead of macros?

I think it is nicer to have mtd_ prefixes, how about these names
instead:

mtd_is_bitflip()
mtd_is_eccerr()
mtd_is_bitflip_or_eccerr()
Artem Bityutskiy - Sept. 19, 2011, 4:14 a.m.
On Sun, 2011-09-11 at 16:57 +0300, Artem Bityutskiy wrote:
> > +#define is_corrected_ecc_error(error) (error == -EUCLEAN)
> 
> This is not really an error...
> 
> > +#define is_uncorrected_ecc_error(error) (error == -EBADMSG)
> > +#define is_ecc_error(error) ((is_corrected_ecc_error(error) || \
> > +			      is_uncorrected_ecc_error(error)))
> 
> Could we please have static inline functions instead of macros?
> 
> I think it is nicer to have mtd_ prefixes, how about these names
> instead:
> 
> mtd_is_bitflip()
> mtd_is_eccerr()
> mtd_is_bitflip_or_eccerr()

Brian, I was actually waiting for v2 of the patches: 3/14, and
9/14-14/14.
Brian Norris - Sept. 19, 2011, 6:43 p.m.
On Sun, Sep 18, 2011 at 9:14 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sun, 2011-09-11 at 16:57 +0300, Artem Bityutskiy wrote:
>> > +#define is_corrected_ecc_error(error) (error == -EUCLEAN)
>>
>> This is not really an error...

I suppose I was just considering the names as describing the error
code, even if it's not fully an error.

>> > +#define is_uncorrected_ecc_error(error) (error == -EBADMSG)
>> > +#define is_ecc_error(error) ((is_corrected_ecc_error(error) || \
>> > +                         is_uncorrected_ecc_error(error)))
>>
>> Could we please have static inline functions instead of macros?

Of course we can! Is there a particular reason? Better type checking?
Or just kernel style? I don't think I see a code-size benefit...

>> I think it is nicer to have mtd_ prefixes, how about these names
>> instead:
>>
>> mtd_is_bitflip()
>> mtd_is_eccerr()
>> mtd_is_bitflip_or_eccerr()
>
> Brian, I was actually waiting for v2 of the patches: 3/14, and
> 9/14-14/14.

Right, I hadn't settled on a name yet and so I haven't rewritten the patches.

I like the fact that your names are shorter. I'm not sure about
`mtd_is_bitflip_or_eccerr()', though; the compound name seems a little
out of place. But maybe it's just better to be clear on exactly what
is covered by the function. Is there a broad category name for
bitflips/ECC errors? I just consider them corrected/uncorrected errors
in flash, thus the naming I gave.

I'll either repost with a better naming scheme or update my patches
with the suggested names.

Brian
Artem Bityutskiy - Sept. 20, 2011, 7:19 a.m.
On Mon, 2011-09-19 at 11:43 -0700, Brian Norris wrote:
> Right, I hadn't settled on a name yet and so I haven't rewritten the patches.
> 
> I like the fact that your names are shorter. I'm not sure about
> `mtd_is_bitflip_or_eccerr()', though; the compound name seems a little
> out of place. But maybe it's just better to be clear on exactly what
> is covered by the function. Is there a broad category name for
> bitflips/ECC errors? I just consider them corrected/uncorrected errors
> in flash, thus the naming I gave.
> 
> I'll either repost with a better naming scheme or update my patches
> with the suggested names.

OK, let's do this sooner than later please, to make sure I have all your
work in my tree, which is safer (consider the situation when dwmw2 picks
what I have now and upstreams before we put everything in).
Brian Norris - Sept. 21, 2011, 1:40 a.m.
On Tue, Sep 20, 2011 at 12:19 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> I'll either repost with a better naming scheme or update my patches
>> with the suggested names.
>
> OK, let's do this sooner than later please, to make sure I have all your
> work in my tree, which is safer (consider the situation when dwmw2 picks
> what I have now and upstreams before we put everything in).

OK, I sent the necessary v2 patches. I just went with your names for
lack of a better idea.

I didn't send v2 of patches 13/14 and 14/14, since there was no
dependency. Let me know if I need to resend anything else.

Brian
Artem Bityutskiy - Sept. 21, 2011, 6:24 a.m.
On Tue, 2011-09-20 at 18:40 -0700, Brian Norris wrote:
> On Tue, Sep 20, 2011 at 12:19 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >> I'll either repost with a better naming scheme or update my patches
> >> with the suggested names.
> >
> > OK, let's do this sooner than later please, to make sure I have all your
> > work in my tree, which is safer (consider the situation when dwmw2 picks
> > what I have now and upstreams before we put everything in).
> 
> OK, I sent the necessary v2 patches. I just went with your names for
> lack of a better idea.
> 
> I didn't send v2 of patches 13/14 and 14/14, since there was no
> dependency. Let me know if I need to resend anything else.

Pushed, thanks for great job!

Patch

diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index ead70ed..1dd74f4 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -348,4 +348,9 @@  void *mtd_kmalloc_up_to(const struct mtd_info *mtd, size_t *size);
 
 void mtd_erase_callback(struct erase_info *instr);
 
+#define is_corrected_ecc_error(error) (error == -EUCLEAN)
+#define is_uncorrected_ecc_error(error) (error == -EBADMSG)
+#define is_ecc_error(error) ((is_corrected_ecc_error(error) || \
+			      is_uncorrected_ecc_error(error)))
+
 #endif /* __MTD_MTD_H__ */