linux-kernel - Re: [PATCH] i2c: xgene-slimpro: Support v2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1509219424.10233.26.camel@linux.intel.com>
Date:   Sat, 28 Oct 2017 22:37:04 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Wolfram Sang <wsa@...-dreams.de>, Hoan Tran <hotran@....com>
Cc:     linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        Loc Ho <lho@....com>, patches@....com
Subject: Re: [PATCH] i2c: xgene-slimpro: Support v2

On Sat, 2017-10-28 at 14:11 +0200, Wolfram Sang wrote:
> Thanks for the patch!
> 
> On Mon, Oct 23, 2017 at 03:12:20PM -0700, Hoan Tran wrote:
> > This patch supports xgene-slimpro-i2c v2 which uses the non-cachable 
> > memory
> > as the PCC shared memory.
> > 
> > Signed-off-by: Hoan Tran <hotran@....com>
> 
> I can't tell if __force is justified here but I don't know much about
> ACPI... Andy, can you have a look?

Few comments to the patch as well.

> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id xgene_slimpro_i2c_acpi_ids[] = {
> > +	{"APMC0D40", XGENE_SLIMPRO_I2C_V1},
> > +	{"APMC0D8B", XGENE_SLIMPRO_I2C_V2},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(acpi, xgene_slimpro_i2c_acpi_ids);
> > +#endif

No need to move this here...

> > +
> >  static int xgene_slimpro_i2c_probe(struct platform_device *pdev)
> >  {
> >  	struct slimpro_i2c_dev *ctx;
> > @@ -476,6 +490,17 @@ static int xgene_slimpro_i2c_probe(struct
> > platform_device *pdev)
> >  		}
> >  	} else {
> >  		struct acpi_pcct_hw_reduced *cppc_ss;
> > +		int version = XGENE_SLIMPRO_I2C_V1;
> > +#ifdef CONFIG_ACPI

Do you really need this ifdef?

> > +		const struct acpi_device_id *acpi_id;
> > +
> > +		acpi_id =
> > acpi_match_device(xgene_slimpro_i2c_acpi_ids,

... you may get pointer to id table via pdev.

> > +					    &pdev->dev);
> > +		if (!acpi_id)
> > +			return -EINVAL;
> > +
> > +		version = (int)acpi_id->driver_data;
> > +#endif

> >  		if (ctx->comm_base_addr) {
> > -			ctx->pcc_comm_addr = memremap(ctx-
> > >comm_base_addr,
> > -						      cppc_ss-
> > >length,
> > -						      MEMREMAP_WB);
> > +			if (version == XGENE_SLIMPRO_I2C_V2)
> > +				ctx->pcc_comm_addr = (void __force
> > *)ioremap(
> > +							ctx-
> > >comm_base_addr,
> > +							cppc_ss-
> > >length);

Commit message actually doesn't explain why we switch to iomap over
memmap.

(__force here is obviously to suppress __iomap annotation for mapped
memory)

If you need non-cacheable MEMREMAP_WT I guess will do a job.

> > +			else
> > +				ctx->pcc_comm_addr = memremap(
> > +							ctx-
> > >comm_base_addr,
> > +							cppc_ss-
> > >length,
> > +							MEMREMAP_WB
> > );

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy