ahci_xgene: Fix the error print invalid resource for APM X-Gene SoC AHCI SATA Host Controller driver.

This patch fixes the error print invalid resource for the APM X-Gene
SoC AHCI SATA Host Controller driver. This print was due to the fact
that the controller 3 don't have a mux resource. This didn't result
in any errors but the print seems like meaningless.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 drivers/ata/ahci_xgene.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
1.8.2.1

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Sep 12, 2014 at 01:24:08PM +0530, Suman Tripathi wrote:
> This patch fixes the error print invalid resource for the APM X-Gene
> SoC AHCI SATA Host Controller driver. This print was due to the fact
> that the controller 3 don't have a mux resource. This didn't result
> in any errors but the print seems like meaningless.
> 
> Signed-off-by: Loc Ho <lho@apm.com>
> Signed-off-by: Suman Tripathi <stripathi@apm.com>
> ---
>  drivers/ata/ahci_xgene.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
> index 40d0a76..9404db0c 100644
> --- a/drivers/ata/ahci_xgene.c
> +++ b/drivers/ata/ahci_xgene.c
> @@ -434,7 +434,7 @@ static int xgene_ahci_mux_select(struct xgene_ahci_context *ctx)
>  	u32 val;
> 
>  	/* Check for optional MUX resource */
> -	if (IS_ERR(ctx->csr_mux))
> +	if (!ctx->csr_mux)
>  		return 0;

Hmmm?  So, if devm_ioremap_resource() call actually fails, now the
function tries to operation on ERR_PTR() value as a real pointer?

Thanks.

Hello,

On Sun, Sep 14, 2014 at 11:36:51AM +0530, Suman Tripathi wrote:
> We can  maintain same piece (IS_ERR(ctx->csr_mux)), then we can do the
> below instead of NULL ??
> 
> ctx->csr_mux = res ? devm_ioremap_resource(dev, res) : ERR_PTR(-EINVAL);

Setting it to NULL on failure would probably make more sense.  No need
to carry around ERR_PTR() value around.

Thanks.

On Mon, Sep 15, 2014 at 01:10:01PM +0530, Suman Tripathi wrote:
> [suman] : So the posted version is acceptable ? Any others comments on this
> patch ?

I'm suggesting setting ctx->cs_mux to NULL on failure.  IOW,

	if (res) {
		ctx->csr_mux = devm_ioremap_resources();
		if (IS_ERR(ctx->csr_mux)) {
			print warning or something;
			ctx->csr_mux = NULL;
		}
	}

Thanks.

On Mon, Sep 15, 2014 at 04:44:52PM +0900, Tejun Heo wrote:
> On Mon, Sep 15, 2014 at 01:10:01PM +0530, Suman Tripathi wrote:
> > [suman] : So the posted version is acceptable ? Any others comments on this
> > patch ?
> 
> I'm suggesting setting ctx->cs_mux to NULL on failure.  IOW,
> 
> 	if (res) {
> 		ctx->csr_mux = devm_ioremap_resources();
> 		if (IS_ERR(ctx->csr_mux)) {
> 			print warning or something;
> 			ctx->csr_mux = NULL;
> 		}
> 	}

A much better approach is:

	if (res) {
		void __iomem *csr = devm_ioremap_resources();
		if (IS_ERR(csr)) {
			ret = ERR_PTR(csr);
			dev_xxx();
			goto err;
		}

		ctx->csr_mux = csr;
	}

Then you never end up in the situation where csr_mux contains an error
pointer value - and is much more obvious that is the case.