Re: [PATCH 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

Re: [PATCH 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[pekon]

+ linux-omap, as this patch touches arch/arm/mach-omap2/gpmc.c

Hi Ezequiel,

On Sunday 07 September 2014 01:26 AM, Ezequiel Garcia wrote:
> The current code abuses ifdefs to determine if the selected ECC scheme
> is supported by the running kernel. As a result the code is hard to read,
> and it also fails to load as a module.
>
> This commit removes all the ifdefs and instead introduces a function
> omap2_nand_ecc_check() to check if the ECC is supported by using
> IS_ENABLED(CONFIG_xxx).
>
Yup, true.
Thanks for cleaning up OMAP2 NAND driver, but few feedbacks.

Just for record, current version of omap2.c NAND driver has so
many #ifdef because:

(1) Many ECC schemes like HAM1 and BCH4_xx, BCH8_SW are supported
only for compatibility with old platforms like OMAP3. And these legacy
schemes have significant amount of code and data foot-print
(as compared to whole driver), so it does not make sense to keep
driver bulky for all new platforms which do not use legacy ECC schemes.

(2) Specifically, BCH4_SW and BCH8_SW ECC schemes used software library
/lib/bch.c "directly". Due to which if BCH4_SW or BCH8_SW was include
in code, /lib/bch.c was included implicitly.

Later from following following commit nand_bch.c wrapper was used which
took care to exclude /lib/bch.c when CONFIG_MTD_NAND_ECC_BCH was not
defined, but #ifdef continued :-( (My bad, I accept ..)

	commit 32d42a855a6f1445373f3d680e9125344aca294c
	mtd: nand: omap: use drivers/mtd/nand/nand_bch.c wrapper for BCH ECC 
instead of lib/bch.c


> Since IS_ENABLED() is true when a config is =y or =m, this change fixes the
> module so it can be loaded with no issues.
>
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>   drivers/mtd/nand/omap2.c          | 134 +++++++++++++++++++++-----------------
>   include/linux/platform_data/elm.h |  14 ++++
>   2 files changed, 87 insertions(+), 61 deletions(-)

[...]

> @@ -1593,33 +1590,71 @@ static int omap_read_page_bch(struct mtd_info *mtd, struct nand_chip *chip,
>   /**
>    * is_elm_present - checks for presence of ELM module by scanning DT nodes
>    * @omap_nand_info: NAND device structure containing platform data
> - * @bch_type: 0x0=BCH4, 0x1=BCH8, 0x2=BCH16
>    */
> -static int is_elm_present(struct omap_nand_info *info,
> -			struct device_node *elm_node, enum bch_ecc bch_type)
> +static bool is_elm_present(struct omap_nand_info *info,
> +			   struct device_node *elm_node)
>   {
>   	struct platform_device *pdev;
> -	struct nand_ecc_ctrl *ecc = &info->nand.ecc;
> -	int err;
> +
>   	/* check whether elm-id is passed via DT */
>   	if (!elm_node) {
> -		pr_err("nand: error: ELM DT node not found\n");
> -		return -ENODEV;
> +		dev_err(&info->pdev->dev, "ELM devicetree node not found\n");
> +		return false;
>   	}
>   	pdev = of_find_device_by_node(elm_node);
>   	/* check whether ELM device is registered */
>   	if (!pdev) {
> -		pr_err("nand: error: ELM device not found\n");
> -		return -ENODEV;
> +		dev_err(&info->pdev->dev, "ELM device not found\n");
> +		return false;
>   	}

Thanks, for this clean-up of replacing pr_err() to dev_err().
Just that if you can resend this as separate patch then I'll surely Ack it.

[...]

> +static bool omap2_nand_ecc_check(struct omap_nand_info *info,
> +				 struct omap_nand_platform_data	*pdata)
> +{
> +	bool ecc_needs_bch, ecc_needs_omap_bch, ecc_needs_elm;
> +
> +	switch (info->ecc_opt) {
> +	case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
> +	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
> +		ecc_needs_omap_bch = false;
> +		ecc_needs_bch = true;
> +		ecc_needs_elm = false;
> +		break;
> +	case OMAP_ECC_BCH4_CODE_HW:
> +	case OMAP_ECC_BCH8_CODE_HW:
> +	case OMAP_ECC_BCH16_CODE_HW:
> +		ecc_needs_omap_bch = true;
> +		ecc_needs_bch = false;
> +		ecc_needs_elm = true;
> +		break;
> +	default:
> +		ecc_needs_omap_bch = false;
> +		ecc_needs_bch = false;
> +		ecc_needs_elm = false;
> +		break;
> +	}
> +
> +	if (ecc_needs_bch && !IS_ENABLED(CONFIG_MTD_NAND_ECC_BCH)) {
> +		dev_err(&info->pdev->dev,
> +			"CONFIG_MTD_NAND_ECC_BCH not enabled\n");
> +		return false;
> +	}
> +	if (ecc_needs_omap_bch && !IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)) {
> +		dev_err(&info->pdev->dev,
> +			"CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
> +		return false;
> +	}
> +	if (ecc_needs_elm && !is_elm_present(info, pdata->elm_of_node)) {
> +		dev_err(&info->pdev->dev, "ELM not available\n");
> +		return false;
> +	}
> +
> +	return true;
>   }

Actually this new function is not required.
We already have this check in GPMC driver when we select the ecc-scheme.
Plz refer: File: /arch/arm/mach-omap2/gpmc.c
	@@ static int gpmc_probe_nand_child(...)
	...
	/* select ecc-scheme for NAND */

is_elm_enabled() is a misnomer here. It actually does not check
for ELM DTS node, because that check is already done in GPMC driver.
It rather checks if ELM device is correctly binded and driver
"initialized" before NAND driver is probed.

Though ELM is not required during NAND probe because there is no error
checking/correction required during NAND probe, but this again comes
from the history of maintaining back-ward compatibility with board-file
(non DTS) approach. Once we move to all DTS approach may be this can be
removed (but on case-by case basis).

[...]

> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
> index 780d1e9..25d1bca 100644
> --- a/include/linux/platform_data/elm.h
> +++ b/include/linux/platform_data/elm.h
> @@ -42,8 +42,22 @@ struct elm_errorvec {
>   	int error_loc[16];
>   };
>
> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)
>   void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>   		struct elm_errorvec *err_vec);
>   int elm_config(struct device *dev, enum bch_ecc bch_type,
>   	int ecc_steps, int ecc_step_size, int ecc_syndrome_size);
> +#else
> +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
> +		struct elm_errorvec *err_vec)
> +{
> +}
> +
> +int elm_config(struct device *dev, enum bch_ecc bch_type,
> +	int ecc_steps, int ecc_step_size, int ecc_syndrome_size)
> +{
> +	return -ENOSYS;
> +}
> +#endif /* CONFIG_MTD_NAND_ECC_BCH */
> +
>   #endif /* __ELM_H */
>
If I'm not wrong, this is all you need in this patch
empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already
handled in /include/linux/mtd/nand_bch.h

So, after this change, you can most of #ifdefs and IS_ENABLED()
and this patch should be simplified.


with regards, pekon

------------------------
Powered by BigRock.com

Re: [PATCH 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[Ezequiel Garcia]

On 07 Sep 02:40 AM, pekon wrote:
> + linux-omap, as this patch touches arch/arm/mach-omap2/gpmc.c
> 
> Hi Ezequiel,
> 
> On Sunday 07 September 2014 01:26 AM, Ezequiel Garcia wrote:
> >The current code abuses ifdefs to determine if the selected ECC scheme
> >is supported by the running kernel. As a result the code is hard to read,
> >and it also fails to load as a module.
> >
> >This commit removes all the ifdefs and instead introduces a function
> >omap2_nand_ecc_check() to check if the ECC is supported by using
> >IS_ENABLED(CONFIG_xxx).
> >
> Yup, true.
> Thanks for cleaning up OMAP2 NAND driver, but few feedbacks.
> 

First of all, this is no cleaning. The driver fails to load as a module
over here. Unless I'm missed something, ifdef seemed false when CONFIG_xxx=m.

> Just for record, current version of omap2.c NAND driver has so
> many #ifdef because:
> 
[..]

I can't think of any valid excuses for cluttering the code with ifdefs like
this. There are several ways of doing things cleaner, by grouping code
differently.

[..]
> >+	}
> >+	if (ecc_needs_elm && !is_elm_present(info, pdata->elm_of_node)) {
> >+		dev_err(&info->pdev->dev, "ELM not available\n");
> >+		return false;
> >+	}
> >+
> >+	return true;
> >  }
> 
> Actually this new function is not required.
[snip]
> If I'm not wrong, this is all you need in this patch
> empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already
> handled in /include/linux/mtd/nand_bch.h
> 
> So, after this change, you can most of #ifdefs and IS_ENABLED()
> and this patch should be simplified.
> 

OK, I'll take a look. Thanks for the suggestion,
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[Ezequiel Garcia]

On 07 Sep 02:40 AM, pekon wrote:
[..]
> >+static bool omap2_nand_ecc_check(struct omap_nand_info *info,
> >+				 struct omap_nand_platform_data	*pdata)
> >+{
> >+	bool ecc_needs_bch, ecc_needs_omap_bch, ecc_needs_elm;
> >+
> >+	switch (info->ecc_opt) {
> >+	case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:
> >+	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
> >+		ecc_needs_omap_bch = false;
> >+		ecc_needs_bch = true;
> >+		ecc_needs_elm = false;
> >+		break;
> >+	case OMAP_ECC_BCH4_CODE_HW:
> >+	case OMAP_ECC_BCH8_CODE_HW:
> >+	case OMAP_ECC_BCH16_CODE_HW:
> >+		ecc_needs_omap_bch = true;
> >+		ecc_needs_bch = false;
> >+		ecc_needs_elm = true;
> >+		break;
> >+	default:
> >+		ecc_needs_omap_bch = false;
> >+		ecc_needs_bch = false;
> >+		ecc_needs_elm = false;
> >+		break;
> >+	}
> >+
> >+	if (ecc_needs_bch && !IS_ENABLED(CONFIG_MTD_NAND_ECC_BCH)) {
> >+		dev_err(&info->pdev->dev,
> >+			"CONFIG_MTD_NAND_ECC_BCH not enabled\n");
> >+		return false;
> >+	}
> >+	if (ecc_needs_omap_bch && !IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)) {
> >+		dev_err(&info->pdev->dev,
> >+			"CONFIG_MTD_NAND_OMAP_BCH not enabled\n");
> >+		return false;
> >+	}
> >+	if (ecc_needs_elm && !is_elm_present(info, pdata->elm_of_node)) {
> >+		dev_err(&info->pdev->dev, "ELM not available\n");
> >+		return false;
> >+	}
> >+
> >+	return true;
> >  }
> 
> Actually this new function is not required.
> We already have this check in GPMC driver when we select the ecc-scheme.
> Plz refer: File: /arch/arm/mach-omap2/gpmc.c
> 	@@ static int gpmc_probe_nand_child(...)
> 	...
> 	/* select ecc-scheme for NAND */
> 
> is_elm_enabled() is a misnomer here. It actually does not check
> for ELM DTS node, because that check is already done in GPMC driver.
> It rather checks if ELM device is correctly binded and driver
> "initialized" before NAND driver is probed.
> 
> Though ELM is not required during NAND probe because there is no error
> checking/correction required during NAND probe, but this again comes
> from the history of maintaining back-ward compatibility with board-file
> (non DTS) approach. Once we move to all DTS approach may be this can be
> removed (but on case-by case basis).
> 
> [...]
> 
> >diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
> >index 780d1e9..25d1bca 100644
> >--- a/include/linux/platform_data/elm.h
> >+++ b/include/linux/platform_data/elm.h
> >@@ -42,8 +42,22 @@ struct elm_errorvec {
> >  	int error_loc[16];
> >  };
> >
> >+#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)
> >  void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
> >  		struct elm_errorvec *err_vec);
> >  int elm_config(struct device *dev, enum bch_ecc bch_type,
> >  	int ecc_steps, int ecc_step_size, int ecc_syndrome_size);
> >+#else
> >+void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
> >+		struct elm_errorvec *err_vec)
> >+{
> >+}
> >+
> >+int elm_config(struct device *dev, enum bch_ecc bch_type,
> >+	int ecc_steps, int ecc_step_size, int ecc_syndrome_size)
> >+{
> >+	return -ENOSYS;
> >+}
> >+#endif /* CONFIG_MTD_NAND_ECC_BCH */
> >+
> >  #endif /* __ELM_H */
> >
> If I'm not wrong, this is all you need in this patch
> empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already
> handled in /include/linux/mtd/nand_bch.h
> 
> So, after this change, you can most of #ifdefs and IS_ENABLED()
> and this patch should be simplified.
> 

If I understand your proposal correctly you are suggesting to drop the
checks for CONFIG_MTD_NAND_ECC_BCH, CONFIG_MTD_NAND_OMAP_BCH, and the ELM
devicetree node.

However, I'd say that change is even more invasive than this one. This commit
merely replaces the current #ifdefs check with IS_ENABLED and tries to do so
in a cleaner way.

This is done on purpose, to keep the current behavior as much as possible.

In addition, if we don't check for the configs explicitly at probe time,
we would only defer the error until some later point, for instance in the call to
nand_chip->ecc.correct(). I don't think that's user-friendly.

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[pekon]

Hi Ezequiel,

On Sunday 07 September 2014 04:47 AM, Ezequiel Garcia wrote:
> On 07 Sep 02:40 AM, pekon wrote:

>> [...]
>>
>>> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
>>> index 780d1e9..25d1bca 100644
>>> --- a/include/linux/platform_data/elm.h
>>> +++ b/include/linux/platform_data/elm.h
>>> @@ -42,8 +42,22 @@ struct elm_errorvec {
>>>   	int error_loc[16];
>>>   };
>>>
>>> +#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)
>>>   void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>>>   		struct elm_errorvec *err_vec);
>>>   int elm_config(struct device *dev, enum bch_ecc bch_type,
>>>   	int ecc_steps, int ecc_step_size, int ecc_syndrome_size);
>>> +#else
>>> +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>>> +		struct elm_errorvec *err_vec)
>>> +{
>>> +}
>>> +
>>> +int elm_config(struct device *dev, enum bch_ecc bch_type,
>>> +	int ecc_steps, int ecc_step_size, int ecc_syndrome_size)
>>> +{
>>> +	return -ENOSYS;
>>> +}
>>> +#endif /* CONFIG_MTD_NAND_ECC_BCH */
>>> +
>>>   #endif /* __ELM_H */
>>>
>> If I'm not wrong, this is all you need in this patch
>> empty functions for CONFIG_MTD_NAND_ECC_BCH exposed symbols are already
>> handled in /include/linux/mtd/nand_bch.h
>>
>> So, after this change, you can most of #ifdefs and IS_ENABLED()
>> and this patch should be simplified.
>>
>
> If I understand your proposal correctly you are suggesting to drop the
> checks for CONFIG_MTD_NAND_ECC_BCH, CONFIG_MTD_NAND_OMAP_BCH, and the ELM
> devicetree node.
>
Absolutely ..
(1) Just the change marked above handles whether
     CONFIG_MTD_NAND_OMAP_BCH is defined or not.
(2) Same way nand_bch.c takes care whether CONFIG_MTD_NAND_ECC_BCH is
     defined or not.
(3) And gpmc.c @@gpmc_probe_nand_child(...) already checks for
     ELM DTS node.

> However, I'd say that change is even more invasive than this one. This commit
> merely replaces the current #ifdefs check with IS_ENABLED and tries to do so
> in a cleaner way.
>
I would say you can get rid of most of #ifdefs and IS_ENABLED()
in this patch itself. And testing it that should be easy, you just
need to compile with CONFIG_MTD_NAND_xxx_BCH enabled one at a time.

> This is done on purpose, to keep the current behavior as much as possible.
>
> In addition, if we don't check for the configs explicitly at probe time,
> we would only defer the error until some later point, for instance in the call to
> nand_chip->ecc.correct(). I don't think that's user-friendly.
>
As ECC-scheme is selected in GPMC driver based on DTS settings, so
any mis-match is easily handled there.
Moreover, error will occur when we change ECC-scheme on-the-fly,
which is still not supported by framework, and require many other
updates if we plan to support that in near future.
So considering ECC-scheme as static configuration is a safe assumption.

But surely you can drop new check in omap2_nand_ecc_check(), which
is already covered in @@gpmc_probe_nand_child(...)

However, I leave it to you and rogerq@ti.com (as he currently
maintains OMAP NAND driver from TI side) to decide how to go about it.


with regards, pekon

------------------------
Powered by BigRock.com

Re: [PATCH 2/3] nand: omap2: Remove horrible ifdefs to fix module probe

[Ezequiel Garcia]

On 07 Sep 03:05 PM, pekon wrote:
[..]
> As ECC-scheme is selected in GPMC driver based on DTS settings, so
> any mis-match is easily handled there.
> Moreover, error will occur when we change ECC-scheme on-the-fly,
> which is still not supported by framework, and require many other
> updates if we plan to support that in near future.
> So considering ECC-scheme as static configuration is a safe assumption.
> 
> But surely you can drop new check in omap2_nand_ecc_check(), which
> is already covered in @@gpmc_probe_nand_child(...)
> 
> However, I leave it to you and rogerq@ti.com (as he currently
> maintains OMAP NAND driver from TI side) to decide how to go about it.
> 

As far as I can see, your proposal only cover the devicetree-probed case
and it won't work for legacy boards.

Given it seems we still support legacy, I'd say your proposal would break
things.

Not to mention that I still prefer to fail at probe time if the driver cannot
deal with the selected ECC, and that's exactly what  omap2_nand_ecc_check()
does.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar