| Message ID | 1448996309-15220-3-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Tue, Dec 01, 2015 at 11:58:28AM -0700, Jason Gunthorpe wrote: > This does a request_resource under the covers which means tis holds a > lock on the memory range it is using so other drivers cannot grab it. > When doing probing it is important to ensure that other drivers are > not using the same range before tis starts touching it. > > To do this flow the actual struct resource from the device right > through to devm_ioremap_resource. This ensures all the proper resource > meta-data is carried down. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > drivers/char/tpm/tpm_tis.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c > index 0a2d94f3d679..1032855c46b2 100644 > --- a/drivers/char/tpm/tpm_tis.c > +++ b/drivers/char/tpm/tpm_tis.c > @@ -67,14 +67,16 @@ enum tis_defaults { > }; > > struct tpm_info { > - unsigned long start; > - unsigned long len; > + struct resource res; > int irq; > }; > > static struct tpm_info tis_default_info = { > - .start = TIS_MEM_BASE, > - .len = TIS_MEM_LEN, > + .res = { > + .start = TIS_MEM_BASE, > + .end = TIS_MEM_BASE + TIS_MEM_LEN - 1, > + .flags = IORESOURCE_MEM, > + }, > .irq = 0, > }; > > @@ -716,7 +718,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, > chip->acpi_dev_handle = acpi_dev_handle; > #endif > > - chip->vendor.iobase = devm_ioremap(dev, tpm_info->start, tpm_info->len); > + chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res); > if (!chip->vendor.iobase) > return -EIO; > > @@ -899,9 +901,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev, > { > struct tpm_info tpm_info = {}; > acpi_handle acpi_dev_handle = NULL; > + struct resource *res; > > - tpm_info.start = pnp_mem_start(pnp_dev, 0); > - tpm_info.len = pnp_mem_len(pnp_dev, 0); > + res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; > + memcpy(&tpm_info.res, res, sizeof(*res)); I think you can do tpm_info.res = res; here, which IMHO reads nicer and maybe is even more efficient (I don't know much about x86). > if (pnp_irq_valid(pnp_dev, 0)) > tpm_info.irq = pnp_irq(pnp_dev, 0); > @@ -964,12 +969,9 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data) > struct tpm_info *tpm_info = (struct tpm_info *) data; > struct resource res; > > - if (acpi_dev_resource_interrupt(ares, 0, &res)) { > + if (acpi_dev_resource_interrupt(ares, 0, &res)) > tpm_info->irq = res.start; > - } else if (acpi_dev_resource_memory(ares, &res)) { > - tpm_info->start = res.start; > - tpm_info->len = resource_size(&res); > - } > + acpi_dev_resource_memory(ares, &tpm_info->res); > > return 1; > } > @@ -992,6 +994,9 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev) > > acpi_dev_free_resource_list(&resources); > > + if (resource_size(&tpm_info.res) == 0) > + return -ENODEV; > + Does this result in an error message from the upper layers? Best regards Uwe
On Tue, Dec 01, 2015 at 08:22:40PM +0100, Uwe Kleine-König wrote: > here, which IMHO reads nicer and maybe is even more efficient (I don't > know much about x86). Sure > > + if (resource_size(&tpm_info.res) == 0) > > + return -ENODEV; > > + > > Does this result in an error message from the upper layers? I think so, yes. The probe will fail which causes the driver core to report a message. The scenario this triggers is if the acpi stuff doesn't have a mem resource, which is a firmware bug, I think. It could get a dedicated print if that is what you are thinking? - if (resource_size(&tpm_info.res) == 0) + if (tpm_info.res.flags == 0) { + dev_err(&pdev->dev, FW_BUG "no memory resource defined\n"); return -ENODEV; + } if (is_itpm(acpi_dev)) itpm = true; [resource_size is wrong as well since it will return 1 for 0'd struct resource, sigh..] Previously it would try to call devm_ioremap with start/len=0 as the range which should also fails in broadly the same way. So this is just moving the existing failure up. Something was needed because the change to struct resource means the new code would call devm_ioremap_resource with a 0'd resource struct, which is not as safe as start/len=0 as before. Jason ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
Hello Jason, On Tue, Dec 01, 2015 at 12:44:19PM -0700, Jason Gunthorpe wrote: > On Tue, Dec 01, 2015 at 08:22:40PM +0100, Uwe Kleine-König wrote: > > > + if (resource_size(&tpm_info.res) == 0) > > > + return -ENODEV; > > > + > > > > Does this result in an error message from the upper layers? > > I think so, yes. The probe will fail which causes the driver core to > report a message. > > The scenario this triggers is if the acpi stuff doesn't have a mem > resource, which is a firmware bug, I think. It could get a dedicated > print if that is what you are thinking? The issue I saw is: There are three(?) ways the tpm could be bound. If one of the succeeds, the other two are expected to fail. But in this case an error message, that the tpm failed to be bound is at least misleading. Best regards Uwe
On Tue, Dec 01, 2015 at 08:52:17PM +0100, Uwe Kleine-König wrote: > The issue I saw is: There are three(?) ways the tpm could be bound. If > one of the succeeds, the other two are expected to fail. But in this > case an error message, that the tpm failed to be bound is at least > misleading. My expectation is that the platform will never have a device that can be bound to more than one and/or the driver core will prevent it (ie if a PNP and ACPI driver claim the same ID the core should bind the ACPI device only, not bind the ACPI device then downgrade to PNP and try to bind the PNP device) This issue pre-exists this patch. All this patch is doing is forcing the tpm_tis to fail to bind instead of potentially running two drivers on the same iorange at once. The only case where this might not be true is if the user specifies force. In this case, if forcing and there is acpi/pnp tpm at the same address, then there will be a message failing the acpi/pnp bind. I feel that is OK because it does indicate the user has done something very questionable. (there is little reason to use force if acpi already has the tpm at the same address range) Jason ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 0a2d94f3d679..1032855c46b2 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -67,14 +67,16 @@ enum tis_defaults { }; struct tpm_info { - unsigned long start; - unsigned long len; + struct resource res; int irq; }; static struct tpm_info tis_default_info = { - .start = TIS_MEM_BASE, - .len = TIS_MEM_LEN, + .res = { + .start = TIS_MEM_BASE, + .end = TIS_MEM_BASE + TIS_MEM_LEN - 1, + .flags = IORESOURCE_MEM, + }, .irq = 0, }; @@ -716,7 +718,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, chip->acpi_dev_handle = acpi_dev_handle; #endif - chip->vendor.iobase = devm_ioremap(dev, tpm_info->start, tpm_info->len); + chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res); if (!chip->vendor.iobase) return -EIO; @@ -899,9 +901,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev, { struct tpm_info tpm_info = {}; acpi_handle acpi_dev_handle = NULL; + struct resource *res; - tpm_info.start = pnp_mem_start(pnp_dev, 0); - tpm_info.len = pnp_mem_len(pnp_dev, 0); + res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0); + if (!res) + return -ENODEV; + memcpy(&tpm_info.res, res, sizeof(*res)); if (pnp_irq_valid(pnp_dev, 0)) tpm_info.irq = pnp_irq(pnp_dev, 0); @@ -964,12 +969,9 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data) struct tpm_info *tpm_info = (struct tpm_info *) data; struct resource res; - if (acpi_dev_resource_interrupt(ares, 0, &res)) { + if (acpi_dev_resource_interrupt(ares, 0, &res)) tpm_info->irq = res.start; - } else if (acpi_dev_resource_memory(ares, &res)) { - tpm_info->start = res.start; - tpm_info->len = resource_size(&res); - } + acpi_dev_resource_memory(ares, &tpm_info->res); return 1; } @@ -992,6 +994,9 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev) acpi_dev_free_resource_list(&resources); + if (resource_size(&tpm_info.res) == 0) + return -ENODEV; + if (is_itpm(acpi_dev)) itpm = true;
This does a request_resource under the covers which means tis holds a lock on the memory range it is using so other drivers cannot grab it. When doing probing it is important to ensure that other drivers are not using the same range before tis starts touching it. To do this flow the actual struct resource from the device right through to devm_ioremap_resource. This ensures all the proper resource meta-data is carried down. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- drivers/char/tpm/tpm_tis.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)