| Message ID | 20170114000954.17728-1-apronin@chromium.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote: > Resetting TPM while processing a command may lead to issues > on the next boot. Ensure that we don't have any ongoing > commands, and that no further commands can be sent to the chip > by unregistering the device in the shutdown handler. > tpm_chip_unregister() waits for the completion of an ongoing > command, if any, and then clears out chip->ops and unregisters > sysfs entities. Unregistering in a shutdown handler seems very strange, it also waits for userspace things, so I wonder if it could be problematic? Maybe just use down_write(&chip->ops_sem); chip->ops = NULL; up_write(&chip->ops_sem); In the shutdown handler? Jason ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote: > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote: > > Resetting TPM while processing a command may lead to issues > > on the next boot. Ensure that we don't have any ongoing > > commands, and that no further commands can be sent to the chip > > by unregistering the device in the shutdown handler. > > tpm_chip_unregister() waits for the completion of an ongoing > > command, if any, and then clears out chip->ops and unregisters > > sysfs entities. > > Unregistering in a shutdown handler seems very strange, it also waits > for userspace things, so I wonder if it could be problematic? > > Maybe just use > > down_write(&chip->ops_sem); > chip->ops = NULL; > up_write(&chip->ops_sem); > > In the shutdown handler? down_write(&chip->ops_sem) would still wait for completing the initiated writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops(). Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be unregistered first. And the last thing, this driver supports TPM 1.2, but if it was a 2.0 chip, it'd also need to send TPM2_Shutdown(CLEAR) from its shutdown handler (or get an unorderly shutdown and DA counter increment). All these things are handled by tpm_chip_unregister(). I thought about creating a tpm_chip_shutdown routine that could be called from shutdown handlers of the drivers that need it (and I'd do it for every driver, especially in 2.0 case). But decided that it's better to reuse the existing tpm_chip_unregister() that already does what's needed. > > Jason ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote: > On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote: > > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote: > > > Resetting TPM while processing a command may lead to issues > > > on the next boot. Ensure that we don't have any ongoing > > > commands, and that no further commands can be sent to the chip > > > by unregistering the device in the shutdown handler. > > > tpm_chip_unregister() waits for the completion of an ongoing > > > command, if any, and then clears out chip->ops and unregisters > > > sysfs entities. > > > > Unregistering in a shutdown handler seems very strange, it also waits > > for userspace things, so I wonder if it could be problematic? > > > > Maybe just use > > > > down_write(&chip->ops_sem); > > chip->ops = NULL; > > up_write(&chip->ops_sem); > > > > In the shutdown handler? > > down_write(&chip->ops_sem) would still wait for completing the initiated > writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops(). > Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be > unregistered first. Why don't you fix the tpm-sysfs issue but rather misusing tpm_chip_unregister? /Jarkko ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote: > On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote: > > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote: > > > Resetting TPM while processing a command may lead to issues > > > on the next boot. Ensure that we don't have any ongoing > > > commands, and that no further commands can be sent to the chip > > > by unregistering the device in the shutdown handler. > > > tpm_chip_unregister() waits for the completion of an ongoing > > > command, if any, and then clears out chip->ops and unregisters > > > sysfs entities. > > > > Unregistering in a shutdown handler seems very strange, it also waits > > for userspace things, so I wonder if it could be problematic? > > > > Maybe just use > > > > down_write(&chip->ops_sem); > > chip->ops = NULL; > > up_write(&chip->ops_sem); > > > > In the shutdown handler? > > down_write(&chip->ops_sem) would still wait for completing the initiated > writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops(). Yes, but that is a timeout limited wait. unregister waits for sysfs files to be closed which is potentially unbounded. > Yes, but it doesn't wait for sysfs > Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be > unregistered first. Yes, sorry, I should have mentioned that.. Maybe that is too much to fix.. > And the last thing, this driver supports TPM 1.2, but if it was a 2.0 > chip, it'd also need to send TPM2_Shutdown(CLEAR) from its shutdown > handler (or get an unorderly shutdown and DA counter increment). I'm confused - doesn't your system reset the TPM when it reboots? Isn't that required so the firmware starts with known PCRs? Doesn't reset trump unorderly shutdown? In any event that seems like an all-chips problem not a chip specific bug fix? > All these things are handled by tpm_chip_unregister(). I thought about > creating a tpm_chip_shutdown routine that could be called from shutdown > handlers of the drivers that need it (and I'd do it for every driver, > especially in 2.0 case). But decided that it's better to reuse the > existing tpm_chip_unregister() that already does what's needed. If for some reason we need this for every driver then this is probably a better approach - but that seems very, very strange to me. Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Mon, Jan 16, 2017 at 09:19:19AM -0700, Jason Gunthorpe wrote: > On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote: > > On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote: > > > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote: > > > > Resetting TPM while processing a command may lead to issues > > > > on the next boot. Ensure that we don't have any ongoing > > > > commands, and that no further commands can be sent to the chip > > > > by unregistering the device in the shutdown handler. > > > > tpm_chip_unregister() waits for the completion of an ongoing > > > > command, if any, and then clears out chip->ops and unregisters > > > > sysfs entities. > > > > > > Unregistering in a shutdown handler seems very strange, it also waits > > > for userspace things, so I wonder if it could be problematic? > > > > > > Maybe just use > > > > > > down_write(&chip->ops_sem); > > > chip->ops = NULL; > > > up_write(&chip->ops_sem); > > > > > > In the shutdown handler? > > > > down_write(&chip->ops_sem) would still wait for completing the initiated > > writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops(). > > Yes, but that is a timeout limited wait. unregister waits for sysfs > files to be closed which is potentially unbounded. > > > Yes, but it doesn't wait for sysfs > > Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be > > unregistered first. > > Yes, sorry, I should have mentioned that.. Maybe that is too much to > fix.. > If we fix sysfs to go through tpm_try_get_ops, then all we can do for shutdown is indeed something like down_write(&chip->ops_sem); if (chip->ops && chip->flags & TPM_CHIP_FLAG_TPM2) tpm2_shutdown(chip, TPM2_SU_CLEAR); chip->ops = NULL; up_write(&chip->ops_sem); Does that sound like a good plan? If we don't want sysfs to increment/decrement the reference count for the device, we can still make it go through down_write(&chip->ops_sem); if (chip->ops) { ... } up_write(&chip->ops_sem); > > And the last thing, this driver supports TPM 1.2, but if it was a 2.0 > > chip, it'd also need to send TPM2_Shutdown(CLEAR) from its shutdown > > handler (or get an unorderly shutdown and DA counter increment). > > I'm confused - doesn't your system reset the TPM when it reboots? > Isn't that required so the firmware starts with known PCRs? Doesn't > reset trump unorderly shutdown? > That's right, the TPM is reset when the system reboots. However, for TPM 2.0, if it resets w/o Shutdown(CLEAR) first, it will detect it during Startup, and mark as unorderly shutdown. Shutdown(CLEAR) is the signal to the TPM to save its state to nvram and prepare to reset. If it was not done, it is possible that something was not saved (e.g. the DA counter), and the chip correctly marks it as a potential DA problem. > In any event that seems like an all-chips problem not a chip specific > bug fix? > The part about TPM 2.0 Shutdown(CLEAR) above is an all-chip (actually, all-2.0-chip) problem. The part where we prevent TPM from being reset in the middle of a command (potentially) may or may not affect other chips - I simply have no knowledge if it leads to issues anywhere else. > > > All these things are handled by tpm_chip_unregister(). I thought about > > creating a tpm_chip_shutdown routine that could be called from shutdown > > handlers of the drivers that need it (and I'd do it for every driver, > > especially in 2.0 case). But decided that it's better to reuse the > > existing tpm_chip_unregister() that already does what's needed. > > If for some reason we need this for every driver then this is probably > a better approach - but that seems very, very strange to me. As described above, we can do a smaller tpm_chip_shutdown() that the drivers that need it (2.0 or susceptible to issues if reset in the middle of command) can call. I'll do it, if it sounds like the right plan to you. Andrey > > Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Tue, Jan 17, 2017 at 09:58:27AM -0800, Andrey Pronin wrote: > > Yes, sorry, I should have mentioned that.. Maybe that is too much to > > fix.. > > If we fix sysfs to go through tpm_try_get_ops, then all we can do for > shutdown is indeed something like Maybe yes, I also had at one point a thought to push the read side of the ops_sem all the way down to the transmit_cmd level... But that complicates calling shutdown. > down_write(&chip->ops_sem); > if (chip->ops && chip->flags & TPM_CHIP_FLAG_TPM2) > tpm2_shutdown(chip, TPM2_SU_CLEAR); > chip->ops = NULL; > up_write(&chip->ops_sem); > > Does that sound like a good plan? > If we don't want sysfs to increment/decrement the reference count for > the device, we can still make it go through Grabbing the extra kref is harmless.. > > I'm confused - doesn't your system reset the TPM when it reboots? > > Isn't that required so the firmware starts with known PCRs? Doesn't > > reset trump unorderly shutdown? > > > > That's right, the TPM is reset when the system reboots. However, for > TPM 2.0, if it resets w/o Shutdown(CLEAR) first, it will detect it > during Startup, and mark as unorderly shutdown. Shutdown(CLEAR) is > the signal to the TPM to save its state to nvram and prepare to reset. > If it was not done, it is possible that something was not saved (e.g. > the DA counter), and the chip correctly marks it as a potential DA > problem. Okay, that makes sense, and needs to go in a comment someplace! > > > All these things are handled by tpm_chip_unregister(). I thought about > > > creating a tpm_chip_shutdown routine that could be called from shutdown > > > handlers of the drivers that need it (and I'd do it for every driver, > > > especially in 2.0 case). But decided that it's better to reuse the > > > existing tpm_chip_unregister() that already does what's needed. > > > > If for some reason we need this for every driver then this is probably > > a better approach - but that seems very, very strange to me. > > As described above, we can do a smaller tpm_chip_shutdown() that the > drivers that need it (2.0 or susceptible to issues if reset in the > middle of command) can call. > I'll do it, if it sounds like the right plan to you. Yes please.. Is there some way we can have the TPM core do this without requiring the driver to add a shutdown the struct driver? Maybe we could put something in chip->dev->driver? Not sure.. Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Tue, Jan 17, 2017 at 12:27:28PM -0700, Jason Gunthorpe wrote: > On Tue, Jan 17, 2017 at 09:58:27AM -0800, Andrey Pronin wrote: > > > Yes, sorry, I should have mentioned that.. Maybe that is too much to > > > fix.. > > > > If we fix sysfs to go through tpm_try_get_ops, then all we can do for > > shutdown is indeed something like > > Maybe yes, I also had at one point a thought to push the read side of > the ops_sem all the way down to the transmit_cmd level... But that > complicates calling shutdown. > > > down_write(&chip->ops_sem); > > if (chip->ops && chip->flags & TPM_CHIP_FLAG_TPM2) > > tpm2_shutdown(chip, TPM2_SU_CLEAR); > > chip->ops = NULL; > > up_write(&chip->ops_sem); > > > > Does that sound like a good plan? > > If we don't want sysfs to increment/decrement the reference count for > > the device, we can still make it go through > > Grabbing the extra kref is harmless.. > > > > I'm confused - doesn't your system reset the TPM when it reboots? > > > Isn't that required so the firmware starts with known PCRs? Doesn't > > > reset trump unorderly shutdown? > > > > > > > That's right, the TPM is reset when the system reboots. However, for > > TPM 2.0, if it resets w/o Shutdown(CLEAR) first, it will detect it > > during Startup, and mark as unorderly shutdown. Shutdown(CLEAR) is > > the signal to the TPM to save its state to nvram and prepare to reset. > > If it was not done, it is possible that something was not saved (e.g. > > the DA counter), and the chip correctly marks it as a potential DA > > problem. > > Okay, that makes sense, and needs to go in a comment someplace! > > > > > All these things are handled by tpm_chip_unregister(). I thought about > > > > creating a tpm_chip_shutdown routine that could be called from shutdown > > > > handlers of the drivers that need it (and I'd do it for every driver, > > > > especially in 2.0 case). But decided that it's better to reuse the > > > > existing tpm_chip_unregister() that already does what's needed. > > > > > > If for some reason we need this for every driver then this is probably > > > a better approach - but that seems very, very strange to me. > > > > As described above, we can do a smaller tpm_chip_shutdown() that the > > drivers that need it (2.0 or susceptible to issues if reset in the > > middle of command) can call. > > I'll do it, if it sounds like the right plan to you. > > Yes please.. > > Is there some way we can have the TPM core do this without requiring > the driver to add a shutdown the struct driver? > > Maybe we could put something in chip->dev->driver? Not sure.. I can play more with it. We can check in tpm_chip_register() if chip->dev->driver->shutdown is NULL, and, if so, set it to a default handler. Or, do register_reboot_notifier() instead, to avoid messing with struct device_driver from tpm-chip.c. Not sure if that's a consideration at alli - any reason not to mess with those structures? In any case, driver->shutdown or register_reboot_notifier, if we still export that same common tpm_shutdown for those drivers that want to do their custom shutdown handlers and register them through module_driver(), we should be ok. Whatever we do, we should allow the drivers to still send (vendor-specific) commands from their shutdown handlers. At some point, we actually used to have a register_reboot_notifier() in the common tpm-chip.c code to make sure that it is done during shutdown. But it is called before .shutdown, so a driver can't do device-specific things with the device (or it can, but through re-implementing the common transfer routines). That's why I switched to a solution where a driver calls this common handler itself, when it is ready for it. Similarly to what's done for tpm_pm_suspend/resume(). But, yes, setting a default handler through chip->dev->driver might just be good enough. > > Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Tue, Jan 17, 2017 at 12:13:36PM -0800, Andrey Pronin wrote: > > Is there some way we can have the TPM core do this without requiring > > the driver to add a shutdown the struct driver? > > > > Maybe we could put something in chip->dev->driver? Not sure.. > > I can play more with it. We can check in tpm_chip_register() if > chip->dev->driver->shutdown is NULL, and, if so, set it to a default > handler. Or, do register_reboot_notifier() instead, to avoid messing > with struct device_driver from tpm-chip.c. Not sure if that's a > consideration at alli - any reason not to mess with those structures? I think ordering is important here, the TPM core has to do any shutdown before the driver shutdown method. That restriction might entirely preclude using a reboot_notifier. > Whatever we do, we should allow the drivers to still send > (vendor-specific) commands from their shutdown handlers. A vendor specific command should be done via a new core TPM mechanism. I really want to keep access drivers (eg i2c, lpc, spi, etc) out of the buisness of *assuming* they are connected to any specific chip. So, the core should detect chip XYZ and then issue the required vendor-specific command in some way. The driver shutdown would be used to close the access interface in some way. > But, yes, setting a default handler through chip->dev->driver > might just be good enough. Probably the *best* thing would be to add shutdown to 'struct class' in the driver core like suspend/resume? Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Tue, Jan 17, 2017 at 01:59:33PM -0700, Jason Gunthorpe wrote: > On Tue, Jan 17, 2017 at 12:13:36PM -0800, Andrey Pronin wrote: > > > Is there some way we can have the TPM core do this without requiring > > > the driver to add a shutdown the struct driver? > > > > > > Maybe we could put something in chip->dev->driver? Not sure.. > > > > I can play more with it. We can check in tpm_chip_register() if > > chip->dev->driver->shutdown is NULL, and, if so, set it to a default > > handler. Or, do register_reboot_notifier() instead, to avoid messing > > with struct device_driver from tpm-chip.c. Not sure if that's a > > consideration at alli - any reason not to mess with those structures? > > I think ordering is important here, the TPM core has to do any > shutdown before the driver shutdown method. That restriction might > entirely preclude using a reboot_notifier. Actually, reboot_notifier is called before the driver shutdown method, so if TPM core registers it, it does its thing first. However, if the driver wants to send vendor-specific commands (using tpm_send to avoid re-implementing tpm_transmit_cmd on its side), it actually wants to do it in the following order: 1) Send vendor specific commands. 2) Call common tpm_shutdown to send Shutdown(CLEAR) and prevent further access to the device. 3) Close the low-level interface in the device-specific way, if/as needed. And for this sequence reboot_notifier gets in the way. So, I'd still allow the driver to explicitly call tpm_shutdown from its shutdown handler, or signal to tpm_tis_core that it wants the default handler - either by leaving .shutdown = NULL, or, for example, by setting a new flag in tpm_tis_data. > > > Whatever we do, we should allow the drivers to still send > > (vendor-specific) commands from their shutdown handlers. > > A vendor specific command should be done via a new core TPM > mechanism. I really want to keep access drivers (eg i2c, lpc, spi, > etc) out of the buisness of *assuming* they are connected to any > specific chip. > Here I was talking not about tpm_tis_spi or tpm_tis. Those can continue relying on the core, or register the default handler using .shutdown = tpm_shutdown. I'm talking about the driver like tpm_i2c_infineon, which although uses the core, is created for a specific family of chips. So, it can assume that it needs to send vendor-specific commands. > So, the core should detect chip XYZ and then issue the required > vendor-specific command in some way. > Do I get it right that you propose to create this new core tpm mechanism for handling shutdowns? I didn't find anything that'd allow to call vendor-specifc hooks from the core during shutdown, but may be I'm missing something. > The driver shutdown would be used to close the access interface in > some way. > > > But, yes, setting a default handler through chip->dev->driver > > might just be good enough. > > Probably the *best* thing would be to add shutdown to 'struct class' > in the driver core like suspend/resume? > Yes, if that could be added to struct class, and then device_shutdown() would call the class suspend, if the driver suspend is NULL, that'd solve it. Then the core can register the default shutdown in class, and an individual access driver can override it by registering its own shutdown callback. Still, due to the ordering issues discussed above, it should be either/or, not first-driver-then-class, if both exist. So, we still need to export the common tpm_shutdown(). So, I'd suggest to start with that: create and export the common tpm_shutdown() from the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops (and fix sysfs to acquire chip->ops_sem) and let the interested drivers call it. If we end up with having class shutdown, we can also register the default handler through that mechanism. For now, we may or may not register the default callback from core through register_reboot_notifier iff chip->dev->driver->shutdown == NULL. That can be debated further. WDYT? Andrey > > Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Tue, Jan 17, 2017 at 03:00:53PM -0800, Andrey Pronin wrote: > Here I was talking not about tpm_tis_spi or tpm_tis. Those can > continue relying on the core, or register the default handler using > .shutdown = tpm_shutdown. I'm talking about the driver like > tpm_i2c_infineon, which although uses the core, is created for a > specific family of chips. So, it can assume that it needs to send > vendor-specific commands. But this is exactly what I'm talking about, infineon chips come in lots of interface types, and if their legacy i2c interface need a vendor command then likely their chips that use a common interface do as well. Conflating interface and bus is something I have been ripping out over the years.. > > So, the core should detect chip XYZ and then issue the required > > vendor-specific command in some way. > > Do I get it right that you propose to create this new core tpm > mechanism for handling shutdowns? I didn't find anything that'd > allow to call vendor-specifc hooks from the core during shutdown, > but may be I'm missing something. It would be simple to add to the core path: if (chip->id == 1234) tpm_vendor_1234_shutdown(...); We don't need to involve the driver in this. If it becomes a very complex thing down then road then we may need *bus* and *chip* drivers, but for now the 'chip' driver(s) are just inlined in the core code.. But if there is no actual need to do this right now then don't worry about overdesigning things.. > > Probably the *best* thing would be to add shutdown to 'struct class' > > in the driver core like suspend/resume? > > Yes, if that could be added to struct class, and then device_shutdown() > would call the class suspend, if the driver suspend is NULL, that'd > solve it. Won't know if it is possible until someone sends a patch to Greg/etc :) > Then the core can register the default shutdown in class, and an > individual access driver can override it by registering its own > shutdown callback. Still, due to the ordering issues discussed > above, it should be either/or, not first-driver-then-class, if both > exist. First class then driver is the usual model, which is OK for TPM. > So, we still need to export the common tpm_shutdown(). No need to export if no driver is calling it, like I said don't overdesign here, it is trivial to change if someone needs to do something different later. > to start with that: create and export the common tpm_shutdown() from > the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops > (and fix sysfs to acquire chip->ops_sem) and let the interested drivers > call it. I think you should do this and use the reboot_notifier or class->shutdown approach I'm not completely sure why you are worrying about sending a vendor-specific command at shutdown. Do you actually need that? Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Tue, Jan 17, 2017 at 04:22:07PM -0700, Jason Gunthorpe wrote: > On Tue, Jan 17, 2017 at 03:00:53PM -0800, Andrey Pronin wrote: > > > Here I was talking not about tpm_tis_spi or tpm_tis. Those can > > continue relying on the core, or register the default handler using > > .shutdown = tpm_shutdown. I'm talking about the driver like > > tpm_i2c_infineon, which although uses the core, is created for a > > specific family of chips. So, it can assume that it needs to send > > vendor-specific commands. > > But this is exactly what I'm talking about, infineon chips come in > lots of interface types, and if their legacy i2c interface need a > vendor command then likely their chips that use a common interface do > as well. > > Conflating interface and bus is something I have been ripping out over > the years.. > Thanks, Jason. OK, I'll try to follow that path then with my patches. > > > So, the core should detect chip XYZ and then issue the required > > > vendor-specific command in some way. > > > > Do I get it right that you propose to create this new core tpm > > mechanism for handling shutdowns? I didn't find anything that'd > > allow to call vendor-specifc hooks from the core during shutdown, > > but may be I'm missing something. > > It would be simple to add to the core path: > > if (chip->id == 1234) > tpm_vendor_1234_shutdown(...); > > We don't need to involve the driver in this. If it becomes a very > complex thing down then road then we may need *bus* and *chip* > drivers, but for now the 'chip' driver(s) are just inlined in the core > code.. > > But if there is no actual need to do this right now then don't worry > about overdesigning things.. OK, I can live with chip->id specific logic in probe/shutdown, if that's the current approach. > > > > Probably the *best* thing would be to add shutdown to 'struct class' > > > in the driver core like suspend/resume? > > > > Yes, if that could be added to struct class, and then device_shutdown() > > would call the class suspend, if the driver suspend is NULL, that'd > > solve it. > > Won't know if it is possible until someone sends a patch to Greg/etc :) > > > Then the core can register the default shutdown in class, and an > > individual access driver can override it by registering its own > > shutdown callback. Still, due to the ordering issues discussed > > above, it should be either/or, not first-driver-then-class, if both > > exist. > > First class then driver is the usual model, which is OK for TPM. If "first class then driver", then the already existing register_reboot_notifier() can play the role of the class handler, since those hooks are called before drivers' shutdown handlers. > > > So, we still need to export the common tpm_shutdown(). > > No need to export if no driver is calling it, like I said don't > overdesign here, it is trivial to change if someone needs to do > something different later. > So, I started putting together an alternative patch (decided to go with a new patch instead of a new version for this one, since it's no longer limited to Infineon driver). The new patch is just going to do the following down_write(&chip->ops_sem); if (chip->ops) { if (chip->flags & TPM_CHIP_FLAG_TPM2) tpm2_shutdown(chip, TPM2_SU_CLEAR); chip->ops = NULL; } up_write(&chip->ops_sem); on shutdown in registered "reboot notifier". I went through the places that access chip->ops to see what's going on at the moment with protecting them with tpm_try_get_ops(). Here is the current state: - tpm_transmit/tpm_transmit_cmd. Not exported and are only called by the core after tpm_try_get_ops() except for one place in sysfs - pubek_show(). - sysfs also directly accesses chip->ops in cancel_store(), but that routine doesn't seem to be used anywhere. Shall it be just removed? - tpm_get_timeouts. Besides the core is called by xen-tpmfront, but before tpm_chip_register(), so no harm possible as of now. - wait_for_tpm_stat. Besides the core is called by xen-tpmfront. It is called from inside chip->ops handlers only, which presumably can happen only when the ops_sem is hold (once sysfs is fixed). - st33zp24 has it's own wait_for_stat() function that goes directly to chip->ops. It happens inside chip->ops->recv/send (as for Xen), which is fine. And also inside its resume handler, which is fine as long as resume can never happen after shutdown (I believe it's true). So, if I add going through tpm_try_get_ops() to pubek_show and delete cancel_store(), that'll fix sysfs, and be enough for the things to work for now. But it looks a bit brittle. So, before I submit my next patch: Do you think it's ok to leave wait_for_tpm_stat() and tpm_get_timeouts() and just continue be mindful when using those low-level functions? Or, shall we instead move acquiring ops_sem and checking for ops == NULL inside those low-level functions: tpm_transmit, wait_for_tpm_stat, tpm_get_timeout. It then should probably be separated from get_device(), which can be kept inside tpm_try_get_ops(). > > to start with that: create and export the common tpm_shutdown() from > > the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops > > (and fix sysfs to acquire chip->ops_sem) and let the interested drivers > > call it. > > I think you should do this and use the reboot_notifier or > class->shutdown approach > > I'm not completely sure why you are worrying about sending a > vendor-specific command at shutdown. Do you actually need that? > Yes, I do need that to send sleep-control vendor commands to the device in my case during shutdown (as well as suspend and resume). It makes much more sense to send them using tpm_transfer_cmd, which relies on chip->ops, than reimplementing the same functionality in the device driver. Again, I can live with "if (chip->id == 1234)" logic in core to achieve that for now, if that's the chosen course. (Or, just register a device-specific "reboot notifier" with lower priority to be called before the "class-level" shutdown logic.) > > Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Mon, Jan 23, 2017 at 12:02:46PM -0800, Andrey Pronin wrote: > On Tue, Jan 17, 2017 at 04:22:07PM -0700, Jason Gunthorpe wrote: > > On Tue, Jan 17, 2017 at 03:00:53PM -0800, Andrey Pronin wrote: > > > > > Here I was talking not about tpm_tis_spi or tpm_tis. Those can > > > continue relying on the core, or register the default handler using > > > .shutdown = tpm_shutdown. I'm talking about the driver like > > > tpm_i2c_infineon, which although uses the core, is created for a > > > specific family of chips. So, it can assume that it needs to send > > > vendor-specific commands. > > > > But this is exactly what I'm talking about, infineon chips come in > > lots of interface types, and if their legacy i2c interface need a > > vendor command then likely their chips that use a common interface do > > as well. > > > > Conflating interface and bus is something I have been ripping out over > > the years.. > > > > Thanks, Jason. OK, I'll try to follow that path then with my patches. > > > > > So, the core should detect chip XYZ and then issue the required > > > > vendor-specific command in some way. > > > > > > Do I get it right that you propose to create this new core tpm > > > mechanism for handling shutdowns? I didn't find anything that'd > > > allow to call vendor-specifc hooks from the core during shutdown, > > > but may be I'm missing something. > > > > It would be simple to add to the core path: > > > > if (chip->id == 1234) > > tpm_vendor_1234_shutdown(...); > > > > We don't need to involve the driver in this. If it becomes a very > > complex thing down then road then we may need *bus* and *chip* > > drivers, but for now the 'chip' driver(s) are just inlined in the core > > code.. > > > > But if there is no actual need to do this right now then don't worry > > about overdesigning things.. > > OK, I can live with chip->id specific logic in probe/shutdown, if that's > the current approach. > > > > > > > Probably the *best* thing would be to add shutdown to 'struct class' > > > > in the driver core like suspend/resume? > > > > > > Yes, if that could be added to struct class, and then device_shutdown() > > > would call the class suspend, if the driver suspend is NULL, that'd > > > solve it. > > > > Won't know if it is possible until someone sends a patch to Greg/etc :) > > > > > Then the core can register the default shutdown in class, and an > > > individual access driver can override it by registering its own > > > shutdown callback. Still, due to the ordering issues discussed > > > above, it should be either/or, not first-driver-then-class, if both > > > exist. > > > > First class then driver is the usual model, which is OK for TPM. > > If "first class then driver", then the already existing > register_reboot_notifier() can play the role of the class handler, > since those hooks are called before drivers' shutdown handlers. > > > > > > So, we still need to export the common tpm_shutdown(). > > > > No need to export if no driver is calling it, like I said don't > > overdesign here, it is trivial to change if someone needs to do > > something different later. > > > > So, I started putting together an alternative patch (decided to go > with a new patch instead of a new version for this one, since it's > no longer limited to Infineon driver). The new patch is just going > to do the following > down_write(&chip->ops_sem); > if (chip->ops) { > if (chip->flags & TPM_CHIP_FLAG_TPM2) > tpm2_shutdown(chip, TPM2_SU_CLEAR); > chip->ops = NULL; > } > up_write(&chip->ops_sem); > on shutdown in registered "reboot notifier". > > I went through the places that access chip->ops to see what's > going on at the moment with protecting them with tpm_try_get_ops(). > Here is the current state: > > - tpm_transmit/tpm_transmit_cmd. Not exported and are only called > by the core after tpm_try_get_ops() except for one place in > sysfs - pubek_show(). > > - sysfs also directly accesses chip->ops in cancel_store(), but > that routine doesn't seem to be used anywhere. Shall it be > just removed? My bad, need more coffee. Of course, cancel_store() is used. So, should fix that as well. > > - tpm_get_timeouts. Besides the core is called by xen-tpmfront, > but before tpm_chip_register(), so no harm possible as of now. > > - wait_for_tpm_stat. Besides the core is called by xen-tpmfront. > It is called from inside chip->ops handlers only, which presumably > can happen only when the ops_sem is hold (once sysfs is fixed). > > - st33zp24 has it's own wait_for_stat() function that goes directly > to chip->ops. It happens inside chip->ops->recv/send (as for Xen), > which is fine. And also inside its resume handler, which is fine > as long as resume can never happen after shutdown (I believe it's > true). > > So, if I add going through tpm_try_get_ops() to pubek_show and > delete cancel_store(), that'll fix sysfs, and be enough for the things > to work for now. > > But it looks a bit brittle. So, before I submit my next patch: > Do you think it's ok to leave wait_for_tpm_stat() and > tpm_get_timeouts() and just continue be mindful when using those > low-level functions? Or, shall we instead move acquiring ops_sem > and checking for ops == NULL inside those low-level functions: > tpm_transmit, wait_for_tpm_stat, tpm_get_timeout. It then should > probably be separated from get_device(), which can be kept inside > tpm_try_get_ops(). > > > > to start with that: create and export the common tpm_shutdown() from > > > the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops > > > (and fix sysfs to acquire chip->ops_sem) and let the interested drivers > > > call it. > > > > I think you should do this and use the reboot_notifier or > > class->shutdown approach > > > > I'm not completely sure why you are worrying about sending a > > vendor-specific command at shutdown. Do you actually need that? > > > > Yes, I do need that to send sleep-control vendor commands to the > device in my case during shutdown (as well as suspend and resume). > It makes much more sense to send them using tpm_transfer_cmd, which > relies on chip->ops, than reimplementing the same functionality in > the device driver. > > Again, I can live with "if (chip->id == 1234)" logic in core to > achieve that for now, if that's the chosen course. (Or, just > register a device-specific "reboot notifier" with lower priority > to be called before the "class-level" shutdown logic.) > > > > > Jason > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > _______________________________________________ > tpmdd-devel mailing list > tpmdd-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Mon, Jan 23, 2017 at 12:02:46PM -0800, Andrey Pronin wrote: > > But if there is no actual need to do this right now then don't worry > > about overdesigning things.. > > OK, I can live with chip->id specific logic in probe/shutdown, if that's > the current approach. It is where we are heading.. If it becomes prevelent we will need chip and bus drivers. > > First class then driver is the usual model, which is OK for TPM. > > If "first class then driver", then the already existing > register_reboot_notifier() can play the role of the class handler, > since those hooks are called before drivers' shutdown handlers. Okay.. But maybe fire off patch doing this via the device code, as if that is accepted it is better than the reboot handler in terms of guaranteeing ordering/etc. > down_write(&chip->ops_sem); > if (chip->ops) { > if (chip->flags & TPM_CHIP_FLAG_TPM2) > tpm2_shutdown(chip, TPM2_SU_CLEAR); > chip->ops = NULL; > } > up_write(&chip->ops_sem); > on shutdown in registered "reboot notifier". Sure, seems like a good starting point. > I went through the places that access chip->ops to see what's > going on at the moment with protecting them with tpm_try_get_ops(). > Here is the current state: > > - tpm_transmit/tpm_transmit_cmd. Not exported and are only called > by the core after tpm_try_get_ops() except for one place in > sysfs - pubek_show(). Most of sysfs calls functions like tpm_pcr_read_dev, tpm_getcap, etc, etc that boil down the tpm_transmit_cmd, so many more need it than you listed. > But it looks a bit brittle. So, before I submit my next patch: > Do you think it's ok to leave wait_for_tpm_stat() and > tpm_get_timeouts() and just continue be mindful when using those > low-level functions? For xenfront we should someday move it to use TPM_OPS_AUTO_STARTUP and drop the tpm_get_timeouts. I think it is fine to leave those two low level commands as is.. Like I said, it would be more robust to acquire a lock directly in places like transmit_cmd, but I suspect that is too hard to retrofit at this time... > and checking for ops == NULL inside those low-level functions: > tpm_transmit, wait_for_tpm_stat, tpm_get_timeout. It then should > probably be separated from get_device(), which can be kept inside > tpm_try_get_ops(). No sense checking for ops=null if you also can't guarentee the lock is held, it is just confusing. Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Mon, Jan 23, 2017 at 01:39:01PM -0700, Jason Gunthorpe wrote: > On Mon, Jan 23, 2017 at 12:02:46PM -0800, Andrey Pronin wrote: > > > > But if there is no actual need to do this right now then don't worry > > > about overdesigning things.. > > > > OK, I can live with chip->id specific logic in probe/shutdown, if that's > > the current approach. > > It is where we are heading.. If it becomes prevelent we will need chip > and bus drivers. > > > > First class then driver is the usual model, which is OK for TPM. > > > > If "first class then driver", then the already existing > > register_reboot_notifier() can play the role of the class handler, > > since those hooks are called before drivers' shutdown handlers. > > Okay.. But maybe fire off patch doing this via the device code, as if > that is accepted it is better than the reboot handler in terms of > guaranteeing ordering/etc. > You mean introducing shutdown in struct class? Hmm, we can try, but... If we do something in 'struct class' for shutdown, that should probably be modeled after power management, for which class handlers are already supported. So, I looked more closely at how this logic is implemented for suspend in drivers/base/power/main.c. Simplifying to class vs driver handlers, it checks class suspend first. And if it exists, calls it. Otherwise, it calls driver suspend. So, it's "either-or", not "first one, then another". Unless I'm missing something obvious, of course... If we want "first one, then another" it might be better to go the register_reboot_notifier() path, instead of trying to add something inconsistent with how it is handled for power management to the core logic. I suspect those notifiers is the mechanism of choice if we need to do both class-level and device-level handling. And I don't want to implement a class-level handler that'd prevent driver handlers from doing device-specific handling, if it is needed. Even if we reverse the order and do "driver first, and iff it doesn't exist, then class", we'd still need to create and export 'tpm_shutdown' for drivers to use in their custom handlers. Otherwise, there's no way for them to zero out chip->ops and prevent further access to the chip. Thus, after looking at that, I'd still suggest doing one of the following: We can go the register_reboot_notifier() route. Which already explicitly supports priorites for those notify handlers, so one can be sure that lower-prio handlers are called first. And, in any case, all these handlers are called before driver->shutdown routines. So, the ordering is very predictable, and we can design whatever fits our needs with it. Or, again, keep it even simpler, but require changes to all tpm drivers. Create and export 'tpm_shutdown' in the core, and let all the tpm drivers register their own shutdown handlers and call it from there, adding their own logic to it, if they want. Sorry, f we go this way, looks like we made the full cycle. But I only now got to checking what's there in kernel for power management. I don't have strong preferences. In both cases, driver writers need to be aware of the shutdown convention. Either (1) know that there's a registered notifier, and they won't be able to use tpm_transmit from driver->shutdown. Or, (2) know that they need to implement driver->shutdown if they support TPM 2.0 or can have problems if the chip is reset in the middle of command. (1) sounds better (fewer drivers will be affected), but not decisively. > > > down_write(&chip->ops_sem); > > if (chip->ops) { > > if (chip->flags & TPM_CHIP_FLAG_TPM2) > > tpm2_shutdown(chip, TPM2_SU_CLEAR); > > chip->ops = NULL; > > } > > up_write(&chip->ops_sem); > > on shutdown in registered "reboot notifier". > > Sure, seems like a good starting point. > > > I went through the places that access chip->ops to see what's > > going on at the moment with protecting them with tpm_try_get_ops(). > > Here is the current state: > > > > - tpm_transmit/tpm_transmit_cmd. Not exported and are only called > > by the core after tpm_try_get_ops() except for one place in > > sysfs - pubek_show(). > > Most of sysfs calls functions like tpm_pcr_read_dev, tpm_getcap, etc, > etc that boil down the tpm_transmit_cmd, so many more need it than you > listed. > Yes, you're right, of course. I definitely needed more coffee before writing that. I ended up adding tpm_try_get_ops/tpm_put_ops to almost every sysfs call once I started looking into them. > > But it looks a bit brittle. So, before I submit my next patch: > > Do you think it's ok to leave wait_for_tpm_stat() and > > tpm_get_timeouts() and just continue be mindful when using those > > low-level functions? > > For xenfront we should someday move it to use TPM_OPS_AUTO_STARTUP and > drop the tpm_get_timeouts. > > I think it is fine to leave those two low level commands as is.. > > Like I said, it would be more robust to acquire a lock directly in > places like transmit_cmd, but I suspect that is too hard to retrofit > at this time... > > > and checking for ops == NULL inside those low-level functions: > > tpm_transmit, wait_for_tpm_stat, tpm_get_timeout. It then should > > probably be separated from get_device(), which can be kept inside > > tpm_try_get_ops(). > > No sense checking for ops=null if you also can't guarentee the lock is > held, it is just confusing. Oh, absolutely. I just meant calling tpm_try_get_ops(), which does both: acquires the lock and checks that ops != NULL. > > Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Mon, Jan 23, 2017 at 02:19:29PM -0800, Andrey Pronin wrote: > Simplifying to class vs driver handlers, it checks class suspend > first. And if it exists, calls it. Otherwise, it calls driver > suspend. So, it's "either-or", not "first one, then another". > Unless I'm missing something obvious, of course... I belive the typical approach is that the class handler then chains to the driver handler at the proper point in the shutdown flow. (ie this way class code can execute before and after the driver power management method) > if it is needed. Even if we reverse the order and do "driver > first, and iff it doesn't exist, then class", we'd still > need to create and export 'tpm_shutdown' for drivers to use The purpose of the driver shutdown would be to shutdown the bus, not the chip. So it would be called with ops already disabled by the core code, there is never a reason for a bus driver to call tpm_shutdown. > We can go the register_reboot_notifier() route. Which already > explicitly supports priorites for those notify handlers, so one This is probably OK too, but it just feels weird to ignore the basic core code pattern and fall back to notifiers, but I understand it can be hard to make those changes.. But still worth at least one attempt, IMHO. Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Mon, Jan 16, 2017 at 11:33:18AM +0200, Jarkko Sakkinen wrote: > On Fri, Jan 13, 2017 at 04:42:30PM -0800, Andrey Pronin wrote: > > On Fri, Jan 13, 2017 at 05:28:57PM -0700, Jason Gunthorpe wrote: > > > On Fri, Jan 13, 2017 at 04:09:54PM -0800, Andrey Pronin wrote: > > > > Resetting TPM while processing a command may lead to issues > > > > on the next boot. Ensure that we don't have any ongoing > > > > commands, and that no further commands can be sent to the chip > > > > by unregistering the device in the shutdown handler. > > > > tpm_chip_unregister() waits for the completion of an ongoing > > > > command, if any, and then clears out chip->ops and unregisters > > > > sysfs entities. > > > > > > Unregistering in a shutdown handler seems very strange, it also waits > > > for userspace things, so I wonder if it could be problematic? > > > > > > Maybe just use > > > > > > down_write(&chip->ops_sem); > > > chip->ops = NULL; > > > up_write(&chip->ops_sem); > > > > > > In the shutdown handler? > > > > down_write(&chip->ops_sem) would still wait for completing the initiated > > writes, since tpm_write() in tpm-dev.c calls tpm_try_get_ops(). > > Also, tpm-sysfs.c calls chip->ops directly, so sysfs should be > > unregistered first. > > Why don't you fix the tpm-sysfs issue but rather misusing > tpm_chip_unregister? Ignore this. I wasn't following this thread properly. Only now had time read carefully through the discussion. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c index 62ee44e57ddc..0c829fe26561 100644 --- a/drivers/char/tpm/tpm_i2c_infineon.c +++ b/drivers/char/tpm/tpm_i2c_infineon.c @@ -689,14 +689,18 @@ static int tpm_tis_i2c_probe(struct i2c_client *client, return rc; } -static int tpm_tis_i2c_remove(struct i2c_client *client) +static void tpm_tis_i2c_shutdown(struct i2c_client *client) { struct tpm_chip *chip = tpm_dev.chip; tpm_chip_unregister(chip); release_locality(chip, tpm_dev.locality, 1); tpm_dev.client = NULL; +} +static int tpm_tis_i2c_remove(struct i2c_client *client) +{ + tpm_tis_i2c_shutdown(client); return 0; } @@ -704,6 +708,7 @@ static struct i2c_driver tpm_tis_i2c_driver = { .id_table = tpm_tis_i2c_table, .probe = tpm_tis_i2c_probe, .remove = tpm_tis_i2c_remove, + .shutdown = tpm_tis_i2c_shutdown, .driver = { .name = "tpm_i2c_infineon", .pm = &tpm_tis_i2c_ops,
Resetting TPM while processing a command may lead to issues on the next boot. Ensure that we don't have any ongoing commands, and that no further commands can be sent to the chip by unregistering the device in the shutdown handler. tpm_chip_unregister() waits for the completion of an ongoing command, if any, and then clears out chip->ops and unregisters sysfs entities. Signed-off-by: Andrey Pronin <apronin@chromium.org> --- drivers/char/tpm/tpm_i2c_infineon.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)