| Message ID | 1414832495-23609-8-git-send-email-jarkko.sakkinen@linux.intel.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sat, Nov 01, 2014 at 11:01:35AM +0200, Jarkko Sakkinen wrote: > Added own class for TPM devices that is used for TPM 2.0 and onwards. > For TPM1 old device structure is kept for backwards compatibility. > > Each struct tpm_chip represents a character device that is associated > to the tpm device class. I think we should hang back on this untill we can answer a few questions.. I certainly thing both TPM1 and TPM2 should create the class, at the worst only tpm2 uses it for the chardev? > @@ -63,7 +65,7 @@ static int tpm_open(struct inode *inode, struct file *file) > * by the check of is_open variable, which is protected > * by driver_lock. */ > if (test_and_set_bit(0, &chip->is_open)) { > - dev_dbg(chip->dev, "Another process owns this TPM\n"); > + dev_dbg(chip->pdev, "Another process owns this TPM\n"); > return -EBUSY; I actually think these are mostly wrong, and are part of why I think tpm1 should create the class too. Except for the probe and remove function everything should log using the TPM device name (eg tpm0) and not the platform device name, so all of these debug statements should stay chip->dev... And considering the volume of changes it might be better to leave 'dev' as a pointer to the tpm class rather than try and tackle that in this giant patch.. > +static void tpm_dev_release(struct device *dev) > +{ > +} And all of this is upside down, the devm interm stuff should hold a get_device() on the struct device and the kfree should live here. > + chip->cdev.owner = chip->pdev->driver->owner; Is that right? the cdev fops is in this module, not the driver's module.. > + rc = cdev_add(&chip->cdev, chip->dev.devt, 1); Can we/should we re-use the misc dev minor number assigned to TPM for tpm0? To me altering the dev major:minor a far more visible change than moving the 'dev' file in sysfs. udev will handle the latter transparently, but the former will break non-udev systems.. Jason ------------------------------------------------------------------------------
On Sun, Nov 02, 2014 at 02:33:05PM -0700, Jason Gunthorpe wrote: > On Sat, Nov 01, 2014 at 11:01:35AM +0200, Jarkko Sakkinen wrote: > > Added own class for TPM devices that is used for TPM 2.0 and onwards. > > For TPM1 old device structure is kept for backwards compatibility. > > > > Each struct tpm_chip represents a character device that is associated > > to the tpm device class. > > I think we should hang back on this untill we can answer a few > questions.. > > I certainly thing both TPM1 and TPM2 should create the class, at the > worst only tpm2 uses it for the chardev? I used the class 'tpm' only for TPM 2.x because I didn't want to break the binary compatibility for TPM 1.x anyway. In ideal situtation both would be character devices inside the class 'tpm' and there would be sysfs attribute such as 'family' to mark the protocol to be used. > > > @@ -63,7 +65,7 @@ static int tpm_open(struct inode *inode, struct file *file) > > * by the check of is_open variable, which is protected > > * by driver_lock. */ > > if (test_and_set_bit(0, &chip->is_open)) { > > - dev_dbg(chip->dev, "Another process owns this TPM\n"); > > + dev_dbg(chip->pdev, "Another process owns this TPM\n"); > > return -EBUSY; > > I actually think these are mostly wrong, and are part of why I think > tpm1 should create the class too. > > Except for the probe and remove function everything should log using > the TPM device name (eg tpm0) and not the platform device name, so all > of these debug statements should stay chip->dev... I tend to agree but here applies my previous comment. > And considering the volume of changes it might be better to leave > 'dev' as a pointer to the tpm class rather than try and tackle that in > this giant patch.. Maybe, or maybe I could make the rename a separate patch? It's fairly mechanical, just a matter of replacing string "chip->dev" with "chip->pdev". > > > +static void tpm_dev_release(struct device *dev) > > +{ > > +} > > And all of this is upside down, the devm interm stuff should hold a > get_device() on the struct device and the kfree should live here. Agreed but didn't want diverge from TPM1 sequences (yet!). I think FIXME comment might be appropriate. > > + chip->cdev.owner = chip->pdev->driver->owner; > > Is that right? the cdev fops is in this module, not the driver's > module.. tpm.ko defines the interface but TPM device driver module owns the character device. I think this is right and similar logic is also for example rtc_device_register(). > > + rc = cdev_add(&chip->cdev, chip->dev.devt, 1); > > Can we/should we re-use the misc dev minor number assigned to TPM for > tpm0? > > To me altering the dev major:minor a far more visible change than > moving the 'dev' file in sysfs. udev will handle the latter > transparently, but the former will break non-udev systems.. I could understand in the context of a misc device but don't really when TPM 2.0 devices have their own device class. Using a 'tpm' class would in all cases break non-udev systems because major number is no longer 10 (misc). > Jason /Jarkko ------------------------------------------------------------------------------
On Mon, Nov 03, 2014 at 07:41:01AM +0200, Jarkko Sakkinen wrote: > I used the class 'tpm' only for TPM 2.x because I didn't want to > break the binary compatibility for TPM 1.x anyway. In ideal situtation > both would be character devices inside the class 'tpm' and there would > be sysfs attribute such as 'family' to mark the protocol to be used. You can create the class without moving away from miscdev... Not having the device creates way to much difference that has to be supported, way too messy. > > And considering the volume of changes it might be better to leave > > 'dev' as a pointer to the tpm class rather than try and tackle that in > > this giant patch.. > > Maybe, or maybe I could make the rename a separate patch? Pointer then rename? > It's fairly mechanical, just a matter of replacing string > "chip->dev" with "chip->pdev". Not everyone should be replaced :| > > > + chip->cdev.owner = chip->pdev->driver->owner; > > > > Is that right? the cdev fops is in this module, not the driver's > > module.. > > tpm.ko defines the interface but TPM device driver module owns the > character device. I think this is right and similar logic is also > for example rtc_device_register(). Hmm, yes, I see that in rtc_dev_prepare - but I don't have time to figure out why that might be :) On the surface, it doesn't seem to make sense: The cdev layer never calls into a function that goes to the chip module - all functions go to the fops in tpm.ko, and tmp.ko is (eventually) responsible for ensuring that ops never points to an unloaded module. So why should it care what the driver module is?? > I could understand in the context of a misc device but don't really > when TPM 2.0 devices have their own device class. Using a 'tpm' class > would in all cases break non-udev systems because major number is no > longer 10 (misc). I'm just saying it would be nice to force the major/minor to misc.tpm for tmp0. No idea if that is OK or not. Jason ------------------------------------------------------------------------------
On Mon, 2014-11-03 at 14:38 -0700, Jason Gunthorpe wrote: > On Mon, Nov 03, 2014 at 07:41:01AM +0200, Jarkko Sakkinen wrote: > > > I used the class 'tpm' only for TPM 2.x because I didn't want to > > break the binary compatibility for TPM 1.x anyway. In ideal situtation > > both would be character devices inside the class 'tpm' and there would > > be sysfs attribute such as 'family' to mark the protocol to be used. > > You can create the class without moving away from miscdev... > > Not having the device creates way to much difference that has to be > supported, way too messy. I have to admit that I'm not quite following here but I assume that you restated this below in more verbose way and this is basically the same argument :) > > > And considering the volume of changes it might be better to leave > > > 'dev' as a pointer to the tpm class rather than try and tackle that in > > > this giant patch.. > > > > Maybe, or maybe I could make the rename a separate patch? > > Pointer then rename? > > > It's fairly mechanical, just a matter of replacing string > > "chip->dev" with "chip->pdev". > > Not everyone should be replaced :| I think the current variable name "dev" is miss-leading. The use of "pdev" would document better the appropriate use for that field (i.e. for the most cases DON'T use it). > > > > + chip->cdev.owner = chip->pdev->driver->owner; > > > > > > Is that right? the cdev fops is in this module, not the driver's > > > module.. > > > > tpm.ko defines the interface but TPM device driver module owns the > > character device. I think this is right and similar logic is also > > for example rtc_device_register(). > > Hmm, yes, I see that in rtc_dev_prepare - but I don't have time to > figure out why that might be :) > > On the surface, it doesn't seem to make sense: The cdev layer never > calls into a function that goes to the chip module - all functions go > to the fops in tpm.ko, and tmp.ko is (eventually) responsible for > ensuring that ops never points to an unloaded module. > > So why should it care what the driver module is?? Fully agreed, I think you got a point here. I think it makes sense as a guideline to kind of "centralize" robustness to tpm.ko and leave as little responsibility as possible to device drivers. > > I could understand in the context of a misc device but don't really > > when TPM 2.0 devices have their own device class. Using a 'tpm' class > > would in all cases break non-udev systems because major number is no > > longer 10 (misc). > > I'm just saying it would be nice to force the major/minor to misc.tpm > for tmp0. > > No idea if that is OK or not. I think that would be a mess. The way things are done in this v5 patch set is actually quite coherent and it does not break backwards compatibility because the "proper" device hierarchy is only used for TPM 2.0 devices. I think the improvement for v6 would be to add a sysctl attribute to disable legacy device hierarchy and sysfs attributes. If that attribute is unset, the old misc hierarchy would be used for any TPM version and TPM 1.0 devices would use existing sysfs attributes. If the flag is set, then the new proper device hierarchy would be used for any TPM device and we would have also chance to redefine sysfs attributes. This would add some legacy clutter to the stack but would be a piss-off-free and non-ABI-breaking solution. > Jason PS. I feel that drivers/char/tpm is a like a desolated town. How many of the people currently marked as maintainers are actually participating to the subsystem development let alone driving it? This subsystem is increasingly important to us given that major software platforms like Chrome OS make extensive use of it and I've heard that even bug fixes have taken some times over a year to get through. Does not look good at all. /Jarkko ------------------------------------------------------------------------------
On Tue, Nov 04, 2014 at 01:47:34PM +0200, Jarkko Sakkinen wrote: > On Mon, 2014-11-03 at 14:38 -0700, Jason Gunthorpe wrote: > > On Mon, Nov 03, 2014 at 07:41:01AM +0200, Jarkko Sakkinen wrote: > > > > > I used the class 'tpm' only for TPM 2.x because I didn't want to > > > break the binary compatibility for TPM 1.x anyway. In ideal situtation > > > both would be character devices inside the class 'tpm' and there would > > > be sysfs attribute such as 'family' to mark the protocol to be used. > > > > You can create the class without moving away from miscdev... > > > > Not having the device creates way to much difference that has to be > > supported, way too messy. > > I have to admit that I'm not quite following here but I assume that > you restated this below in more verbose way and this is basically the > same argument :) > > > > > And considering the volume of changes it might be better to leave > > > > 'dev' as a pointer to the tpm class rather than try and tackle that in > > > > this giant patch.. > > > > > > Maybe, or maybe I could make the rename a separate patch? > > > > Pointer then rename? > > > > > It's fairly mechanical, just a matter of replacing string > > > "chip->dev" with "chip->pdev". > > > > Not everyone should be replaced :| > > I think the current variable name "dev" is miss-leading. The > use of "pdev" would document better the appropriate use for that > field (i.e. for the most cases DON'T use it). > > > > > > + chip->cdev.owner = chip->pdev->driver->owner; > > > > > > > > Is that right? the cdev fops is in this module, not the driver's > > > > module.. > > > > > > tpm.ko defines the interface but TPM device driver module owns the > > > character device. I think this is right and similar logic is also > > > for example rtc_device_register(). > > > > Hmm, yes, I see that in rtc_dev_prepare - but I don't have time to > > figure out why that might be :) > > > > On the surface, it doesn't seem to make sense: The cdev layer never > > calls into a function that goes to the chip module - all functions go > > to the fops in tpm.ko, and tmp.ko is (eventually) responsible for > > ensuring that ops never points to an unloaded module. > > > > So why should it care what the driver module is?? > > Fully agreed, I think you got a point here. I think it makes sense > as a guideline to kind of "centralize" robustness to tpm.ko and > leave as little responsibility as possible to device drivers. > > > > I could understand in the context of a misc device but don't really > > > when TPM 2.0 devices have their own device class. Using a 'tpm' class > > > would in all cases break non-udev systems because major number is no > > > longer 10 (misc). > > > > I'm just saying it would be nice to force the major/minor to misc.tpm > > for tmp0. > > > > No idea if that is OK or not. > > I think that would be a mess. The way things are done in this v5 > patch set is actually quite coherent and it does not break backwards > compatibility because the "proper" device hierarchy is only used for > TPM 2.0 devices. > > I think the improvement for v6 would be to add a sysctl attribute > to disable legacy device hierarchy and sysfs attributes. If that > attribute is unset, the old misc hierarchy would be used for any > TPM version and TPM 1.0 devices would use existing sysfs attributes. Mistake here. Should be a module parameter to be not racy, not sysctl attribute :) > If the flag is set, then the new proper device hierarchy would be > used for any TPM device and we would have also chance to redefine > sysfs attributes. > > This would add some legacy clutter to the stack but would be a > piss-off-free and non-ABI-breaking solution. > > > Jason > > PS. I feel that drivers/char/tpm is a like a desolated town. How > many of the people currently marked as maintainers are actually > participating to the subsystem development let alone driving it? > > This subsystem is increasingly important to us given that major > software platforms like Chrome OS make extensive use of it and > I've heard that even bug fixes have taken some times over a year > to get through. Does not look good at all. /Jarkko ------------------------------------------------------------------------------
On Tue, Nov 04, 2014 at 01:47:34PM +0200, Jarkko Sakkinen wrote: > > > I used the class 'tpm' only for TPM 2.x because I didn't want to > > > break the binary compatibility for TPM 1.x anyway. In ideal situtation > > > both would be character devices inside the class 'tpm' and there would > > > be sysfs attribute such as 'family' to mark the protocol to be used. > > > > You can create the class without moving away from miscdev... > > > > Not having the device creates way to much difference that has to be > > supported, way too messy. > > I have to admit that I'm not quite following here but I assume that > you restated this below in more verbose way and this is basically the > same argument :) I mean, if we have a patch that does: struct tpm_chip { struct device cdev; // the class device struct device *pdev; // the 'platform' device chip is bound too struct device *dev = pdev; // Temporary Compatability [+ device_register/etc/etc] Then both cdev and pdev should always be valid. We should not have cdev be valid for TPM2 and invalid for TPM1, that is just a big mess. Just this change is perfectly safe, it creates the /sys/class/tpm/tpm0 directory, but doesn't change anything already existing Then, a patch: go through and replace all uses of chip->dev with &chip->cdev in 90% of cases Another patch: use chip->pdev for the handfull of the rest, and drop dev entirely Then a patch: Drop misc_register entirely, no compat stuff. Explain clearly the resulting sysfs changes, CC the various people who monitor the sysfs API, act on any feedback. I'm hoping it is an OK change. [ If it is not OK then we can talk about using it only for TPM2 or whatever ] > I think the current variable name "dev" is miss-leading. The > use of "pdev" would document better the appropriate use for that > field (i.e. for the most cases DON'T use it). Yes, see above :) > I think that would be a mess. The way things are done in this v5 > patch set is actually quite coherent and it does not break backwards > compatibility because the "proper" device hierarchy is only used for > TPM 2.0 devices. I mean, just do something like this: if (chip->id == 0) chip->cdev.devt = MKDEV(MISC_MAJOR, TPM_MINOR) else chip->cdev.devt = [.. dynamic alloc_chrddev_region ..] Very simple, keeps the major/minor for TPM0, moves the 'dev' file to the new location in sysfs. I can't find another example of this in the kernel, so I don't know what the thinking is.. Jason ------------------------------------------------------------------------------
On Tue, Nov 04, 2014 at 11:14:33AM -0700, Jason Gunthorpe wrote: > On Tue, Nov 04, 2014 at 01:47:34PM +0200, Jarkko Sakkinen wrote: > > > > > I used the class 'tpm' only for TPM 2.x because I didn't want to > > > > break the binary compatibility for TPM 1.x anyway. In ideal situtation > > > > both would be character devices inside the class 'tpm' and there would > > > > be sysfs attribute such as 'family' to mark the protocol to be used. > > > > > > You can create the class without moving away from miscdev... > > > > > > Not having the device creates way to much difference that has to be > > > supported, way too messy. > > > > I have to admit that I'm not quite following here but I assume that > > you restated this below in more verbose way and this is basically the > > same argument :) > > I mean, if we have a patch that does: > > struct tpm_chip { > struct device cdev; // the class device > struct device *pdev; // the 'platform' device chip is bound too > > struct device *dev = pdev; // Temporary Compatability > [+ device_register/etc/etc] > > Then both cdev and pdev should always be valid. We should not have cdev > be valid for TPM2 and invalid for TPM1, that is just a big mess. > > Just this change is perfectly safe, it creates the /sys/class/tpm/tpm0 > directory, but doesn't change anything already existing > > Then, a patch: go through and replace all uses of chip->dev with &chip->cdev in > 90% of cases > > Another patch: use chip->pdev for the handfull of the rest, and drop > dev entirely > > Then a patch: Drop misc_register entirely, no compat stuff. Explain > clearly the resulting sysfs changes, CC the various people who monitor > the sysfs API, act on any feedback. I'm hoping it is an OK change. > [ If it is not OK then we can talk about using it only for TPM2 or > whatever ] Hold on. So you are proposing that for a TPM 1.0 device you would have simultaneously as an intermediate step: - /sys/class/tpm/tpm0 - /sys/class/misc/tpm0 Maybe it would be a better idea to just create patch that would simply - wipe /sys/class/misc/tpm0 - introduce /sys/class/tpm/tpm0 - for TPM 1 put existing sysfs attributes /sys/class/tpm/tpm0 I would put this into a single patch. I don't really like those intermediate steps. I think they cause more confusion but I think as a whole the change is reasonable. I'll do this "aggressive" patch for v6 and "soften" it based on feedback if it is not liked (and CC to appropriate people). > > I think the current variable name "dev" is miss-leading. The > > use of "pdev" would document better the appropriate use for that > > field (i.e. for the most cases DON'T use it). > > Yes, see above :) > > > I think that would be a mess. The way things are done in this v5 > > patch set is actually quite coherent and it does not break backwards > > compatibility because the "proper" device hierarchy is only used for > > TPM 2.0 devices. > > I mean, just do something like this: > > if (chip->id == 0) > chip->cdev.devt = MKDEV(MISC_MAJOR, TPM_MINOR) > else > chip->cdev.devt = [.. dynamic alloc_chrddev_region ..] > > Very simple, keeps the major/minor for TPM0, moves the 'dev' file to > the new location in sysfs. > > I can't find another example of this in the kernel, so I don't know > what the thinking is.. Maybe it would be reasonable to break the path structure and do this change for the benefit of backward compatibility. Not doing the change your proposed might really cause unwanted breakage. Moving the current TPM sysfs attributes is not really an issue because they are only for human consumption (not machine readable in any sany way) :) And AFAIK TrouSerS does not use sysfs PPI interface. > Jason /Jarkko ------------------------------------------------------------------------------
On Tue, Nov 04, 2014 at 10:38:43PM +0200, Jarkko Sakkinen wrote: > > Then a patch: Drop misc_register entirely, no compat stuff. Explain > > clearly the resulting sysfs changes, CC the various people who monitor > > the sysfs API, act on any feedback. I'm hoping it is an OK change. > > [ If it is not OK then we can talk about using it only for TPM2 or > > whatever ] > > Hold on. So you are proposing that for a TPM 1.0 device you would > have simultaneously as an intermediate step: > > - /sys/class/tpm/tpm0 > - /sys/class/misc/tpm0 Yes, well more specifically: /sys/class/tpm/tpm0/ /sys/class/misc/tpm0/dev The reason for this is simply that adding to sysfs is certainly OK, I don't think the tpm and misc device will collide in any way - udev will look for the 'dev' file and ignore the class/tpm directory. It breaks up the patch too, this would be a monster: > Maybe it would be a better idea to just create patch that would simply Because of all the variable rename noise and so forth. Even if the net result is we apply several patchs in a row that wipes /sys/class/misc/tpm0 the patches themselves will be smaller and more reviewable if split, especially those big variable rename ones.. > Moving the current TPM sysfs attributes is not really an issue because > they are only for human consumption (not machine readable in any sany > way) :) And AFAIK TrouSerS does not use sysfs PPI interface. Someone in user space must use that event log and PPI stuff??? Does anyone know who and how? Jason ------------------------------------------------------------------------------
On Tue, Nov 04, 2014 at 01:49:18PM -0700, Jason Gunthorpe wrote: > On Tue, Nov 04, 2014 at 10:38:43PM +0200, Jarkko Sakkinen wrote: > > > > Then a patch: Drop misc_register entirely, no compat stuff. Explain > > > clearly the resulting sysfs changes, CC the various people who monitor > > > the sysfs API, act on any feedback. I'm hoping it is an OK change. > > > [ If it is not OK then we can talk about using it only for TPM2 or > > > whatever ] > > > > Hold on. So you are proposing that for a TPM 1.0 device you would > > have simultaneously as an intermediate step: > > > > - /sys/class/tpm/tpm0 > > - /sys/class/misc/tpm0 > > Yes, well more specifically: > > /sys/class/tpm/tpm0/ > /sys/class/misc/tpm0/dev > > The reason for this is simply that adding to sysfs is certainly OK, > I don't think the tpm and misc device will collide in any way - udev > will look for the 'dev' file and ignore the class/tpm directory. > > It breaks up the patch too, this would be a monster: > > > Maybe it would be a better idea to just create patch that would simply > > Because of all the variable rename noise and so forth. > > Even if the net result is we apply several patchs in a row that wipes > /sys/class/misc/tpm0 the patches themselves will be smaller and more > reviewable if split, especially those big variable rename ones.. OK, I see your point. I'll break it up into more reasonable steps. > > Moving the current TPM sysfs attributes is not really an issue because > > they are only for human consumption (not machine readable in any sany > > way) :) And AFAIK TrouSerS does not use sysfs PPI interface. > > Someone in user space must use that event log and PPI stuff??? Does > anyone know who and how? > > Jason /Jarkko ------------------------------------------------------------------------------
On Tue, Nov 04, 2014 at 11:14:33AM -0700, Jason Gunthorpe wrote: > On Tue, Nov 04, 2014 at 01:47:34PM +0200, Jarkko Sakkinen wrote: > > > > > I used the class 'tpm' only for TPM 2.x because I didn't want to > > > > break the binary compatibility for TPM 1.x anyway. In ideal situtation > > > > both would be character devices inside the class 'tpm' and there would > > > > be sysfs attribute such as 'family' to mark the protocol to be used. > > > > > > You can create the class without moving away from miscdev... > > > > > > Not having the device creates way to much difference that has to be > > > supported, way too messy. > > > > I have to admit that I'm not quite following here but I assume that > > you restated this below in more verbose way and this is basically the > > same argument :) > > I mean, if we have a patch that does: > > struct tpm_chip { > struct device cdev; // the class device > struct device *pdev; // the 'platform' device chip is bound too > > struct device *dev = pdev; // Temporary Compatability > [+ device_register/etc/etc] > > Then both cdev and pdev should always be valid. We should not have cdev > be valid for TPM2 and invalid for TPM1, that is just a big mess. As a first patch I'll do a patch that does dev -> pdev rename and nothing else. IMHO it's clean and easy to review if no other changes are contained. One reason for this is obviously that I want to use cdev for struct cdev not for the class device. > Just this change is perfectly safe, it creates the /sys/class/tpm/tpm0 > directory, but doesn't change anything already existing > > Then, a patch: go through and replace all uses of chip->dev with &chip->cdev in > 90% of cases > > Another patch: use chip->pdev for the handfull of the rest, and drop > dev entirely > > Then a patch: Drop misc_register entirely, no compat stuff. Explain > clearly the resulting sysfs changes, CC the various people who monitor > the sysfs API, act on any feedback. I'm hoping it is an OK change. > [ If it is not OK then we can talk about using it only for TPM2 or > whatever ] > > > I think the current variable name "dev" is miss-leading. The > > use of "pdev" would document better the appropriate use for that > > field (i.e. for the most cases DON'T use it). > > Yes, see above :) > > > I think that would be a mess. The way things are done in this v5 > > patch set is actually quite coherent and it does not break backwards > > compatibility because the "proper" device hierarchy is only used for > > TPM 2.0 devices. > > I mean, just do something like this: > > if (chip->id == 0) > chip->cdev.devt = MKDEV(MISC_MAJOR, TPM_MINOR) > else > chip->cdev.devt = [.. dynamic alloc_chrddev_region ..] > > Very simple, keeps the major/minor for TPM0, moves the 'dev' file to > the new location in sysfs. > > I can't find another example of this in the kernel, so I don't know > what the thinking is.. > > Jason ------------------------------------------------------------------------------
On Wed, Nov 05, 2014 at 09:40:29AM +0200, Jarkko Sakkinen wrote: > > I mean, if we have a patch that does: > > > > struct tpm_chip { > > struct device cdev; // the class device > > struct device *pdev; // the 'platform' device chip is bound too > > > > struct device *dev = pdev; // Temporary Compatability > > [+ device_register/etc/etc] > > > > Then both cdev and pdev should always be valid. We should not have cdev > > be valid for TPM2 and invalid for TPM1, that is just a big mess. > As a first patch I'll do a patch that does dev -> pdev rename and > nothing else. IMHO it's clean and easy to review if no other changes > are contained. One reason for this is obviously that I want to use > cdev for struct cdev not for the class device. Well, once you add cdev, pdev and dev, you want most uses of dev to become cdev and some uses to become pdev. Just bulk renaming dev -> pdev and then bulk renaming pdev -> cdev seems like lots of churn... Jason ------------------------------------------------------------------------------
On Wed, Nov 05, 2014 at 10:48:36AM -0700, Jason Gunthorpe wrote: > On Wed, Nov 05, 2014 at 09:40:29AM +0200, Jarkko Sakkinen wrote: > > > > I mean, if we have a patch that does: > > > > > > struct tpm_chip { > > > struct device cdev; // the class device > > > struct device *pdev; // the 'platform' device chip is bound too > > > > > > struct device *dev = pdev; // Temporary Compatability > > > [+ device_register/etc/etc] > > > > > > Then both cdev and pdev should always be valid. We should not have cdev > > > be valid for TPM2 and invalid for TPM1, that is just a big mess. > > > As a first patch I'll do a patch that does dev -> pdev rename and > > nothing else. IMHO it's clean and easy to review if no other changes > > are contained. One reason for this is obviously that I want to use > > cdev for struct cdev not for the class device. > > Well, once you add cdev, pdev and dev, you want most uses of dev to > become cdev and some uses to become pdev. struct cdev uses variable name 'cdev' in my previous patch set so this naming scheme doesn't work. I do not understand why dev -> pdev rename as the very first commit would be such a bad thing. It's anyway better variable name for it. After that variable name 'dev' is free to be used for class device. > Just bulk renaming dev -> pdev and then bulk renaming pdev -> cdev > seems like lots of churn... I'm not planning to do such thing. In the end there is: struct device dev; struct device *pdev; struct cdev cdev; This will require only one "rename commit". What I'm going to take from your feedback is to make /dev/tpm0 special case when considering major:minor just like you proposed. It's very good idea. Other thing I'm going to do is to leave PPI sysfs interface to tpmX/device/ppi at least for TPM1 devices. I'll bake the next patch set hopefully before the end of this week. Lets see after that if it stil need refinement or breaking up to smaller patches. > Jason /Jarkko ------------------------------------------------------------------------------
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 9d2038b..9cc3ccf 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -45,7 +45,7 @@ struct tpm_chip *tpm_chip_find_get(int chip_num) if (chip_num != TPM_ANY_NUM && chip_num != pos->dev_num) continue; - if (try_module_get(pos->dev->driver->owner)) { + if (try_module_get(pos->pdev->driver->owner)) { chip = pos; break; } @@ -111,7 +111,7 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev, scnprintf(chip->devname, sizeof(chip->devname), "%s%d", "tpm", chip->dev_num); - chip->dev = dev; + chip->pdev = dev; devm_add_action(dev, tpmm_chip_remove, chip); dev_set_drvdata(dev, chip); @@ -140,7 +140,10 @@ int tpm_chip_register(struct tpm_chip *chip) { int rc; - rc = tpm_dev_add_device(chip); + if (chip->flags & TPM_CHIP_FLAG_TPM2) + rc = tpm2_dev_add_device(chip); + else + rc = tpm_dev_add_device(chip); if (rc) return rc; @@ -198,6 +201,9 @@ void tpm_chip_unregister(struct tpm_chip *chip) tpm_sysfs_del_device(chip); } - tpm_dev_del_device(chip); + if (chip->flags & TPM_CHIP_FLAG_TPM2) + tpm2_dev_del_device(chip); + else + tpm_dev_del_device(chip); } EXPORT_SYMBOL_GPL(tpm_chip_unregister); diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c index d9b774e..8c4ab94 100644 --- a/drivers/char/tpm/tpm-dev.c +++ b/drivers/char/tpm/tpm-dev.c @@ -22,6 +22,8 @@ #include <linux/uaccess.h> #include "tpm.h" +static dev_t tpm_devt; + struct file_priv { struct tpm_chip *chip; @@ -63,7 +65,7 @@ static int tpm_open(struct inode *inode, struct file *file) * by the check of is_open variable, which is protected * by driver_lock. */ if (test_and_set_bit(0, &chip->is_open)) { - dev_dbg(chip->dev, "Another process owns this TPM\n"); + dev_dbg(chip->pdev, "Another process owns this TPM\n"); return -EBUSY; } @@ -81,7 +83,7 @@ static int tpm_open(struct inode *inode, struct file *file) INIT_WORK(&priv->work, timeout_work); file->private_data = priv; - get_device(chip->dev); + get_device(chip->pdev); return 0; } @@ -168,7 +170,7 @@ static int tpm_release(struct inode *inode, struct file *file) file->private_data = NULL; atomic_set(&priv->data_pending, 0); clear_bit(0, &priv->chip->is_open); - put_device(priv->chip->dev); + put_device(priv->chip->pdev); kfree(priv); return 0; } @@ -193,12 +195,12 @@ int tpm_dev_add_device(struct tpm_chip *chip) chip->vendor.miscdev.minor = MISC_DYNAMIC_MINOR; chip->vendor.miscdev.name = chip->devname; - chip->vendor.miscdev.parent = chip->dev; + chip->vendor.miscdev.parent = chip->pdev; rc = misc_register(&chip->vendor.miscdev); if (rc) { chip->vendor.miscdev.name = NULL; - dev_err(chip->dev, + dev_err(chip->pdev, "unable to misc_register %s, minor %d err=%d\n", chip->vendor.miscdev.name, chip->vendor.miscdev.minor, rc); @@ -211,3 +213,68 @@ void tpm_dev_del_device(struct tpm_chip *chip) if (chip->vendor.miscdev.name) misc_deregister(&chip->vendor.miscdev); } + +static void tpm_dev_release(struct device *dev) +{ +} + +int tpm2_dev_add_device(struct tpm_chip *chip) +{ + int rc; + + chip->dev.parent = chip->pdev; + chip->dev.class = tpm_class; + chip->dev.release = tpm_dev_release; + chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num); + + dev_set_name(&chip->dev, "tpm%d", chip->dev_num); + + cdev_init(&chip->cdev, &tpm_fops); + + chip->cdev.owner = chip->pdev->driver->owner; + + rc = device_register(&chip->dev); + if (rc) { + + dev_err(chip->pdev, + "tpm: registering %s failed with %d\n", + chip->devname, rc); + put_device(&chip->dev); + return rc; + } + + rc = cdev_add(&chip->cdev, chip->dev.devt, 1); + if (rc) { + dev_err(&chip->dev, "adding cdev for %s failed with %d\n", + chip->devname, rc); + device_unregister(&chip->dev); + put_device(&chip->dev); + return rc; + } + + return 0; +} + +void tpm2_dev_del_device(struct tpm_chip *chip) +{ + if (get_device(&chip->dev) != NULL) { + cdev_del(&chip->cdev); + device_unregister(&chip->dev); + put_device(&chip->dev); + } +} + +void __init tpm_dev_init(void) +{ + int rc; + + rc = alloc_chrdev_region(&tpm_devt, 0, TPM_NUM_DEVICES, "tpm"); + if (rc < 0) + pr_err("tpm: failed to allocate char dev region\n"); +} + +void __exit tpm_dev_exit(void) +{ + if (tpm_devt) + unregister_chrdev_region(tpm_devt, TPM_NUM_DEVICES); +} diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index a98532f..41426fb 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -38,6 +38,8 @@ #define TPM_PROTECTED_COMMAND 0x00 #define TPM_CONNECTION_COMMAND 0x40 +struct class *tpm_class; + /* * Bug workaround - some TPM's don't flush the most * recently changed pcr on suspend, so force the flush @@ -343,7 +345,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, if (count == 0) return -ENODATA; if (count > bufsiz) { - dev_err(chip->dev, + dev_err(chip->pdev, "invalid count value %x %zx\n", count, bufsiz); return -E2BIG; } @@ -352,7 +354,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, rc = chip->ops->send(chip, (u8 *) buf, count); if (rc < 0) { - dev_err(chip->dev, + dev_err(chip->pdev, "tpm_transmit: tpm_send: error %zd\n", rc); goto out; } @@ -371,7 +373,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, goto out_recv; if (chip->ops->req_canceled(chip, status)) { - dev_err(chip->dev, "Operation Canceled\n"); + dev_err(chip->pdev, "Operation Canceled\n"); rc = -ECANCELED; goto out; } @@ -381,14 +383,14 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, } while (time_before(jiffies, stop)); chip->ops->cancel(chip); - dev_err(chip->dev, "Operation Timed out\n"); + dev_err(chip->pdev, "Operation Timed out\n"); rc = -ETIME; goto out; out_recv: rc = chip->ops->recv(chip, (u8 *) buf, bufsiz); if (rc < 0) - dev_err(chip->dev, + dev_err(chip->pdev, "tpm_transmit: tpm_recv: error %zd\n", rc); out: mutex_unlock(&chip->tpm_mutex); @@ -414,7 +416,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, err = be32_to_cpu(header->return_code); if (err != 0 && desc) - dev_err(chip->dev, "A TPM error (%d) occurred %s\n", err, desc); + dev_err(chip->pdev, "A TPM error (%d) occurred %s\n", err, desc); return err; } @@ -508,7 +510,7 @@ int tpm_get_timeouts(struct tpm_chip *chip) if (rc == TPM_ERR_INVALID_POSTINIT) { /* The TPM is not started, we are the first to talk to it. Execute a startup command. */ - dev_info(chip->dev, "Issuing TPM_STARTUP"); + dev_info(chip->pdev, "Issuing TPM_STARTUP"); if (tpm_startup(chip, TPM_ST_CLEAR)) return rc; @@ -520,7 +522,7 @@ int tpm_get_timeouts(struct tpm_chip *chip) NULL); } if (rc) { - dev_err(chip->dev, + dev_err(chip->pdev, "A TPM error (%zd) occurred attempting to determine the timeouts\n", rc); goto duration; @@ -559,7 +561,7 @@ int tpm_get_timeouts(struct tpm_chip *chip) /* Report adjusted timeouts */ if (chip->vendor.timeout_adjusted) { - dev_info(chip->dev, + dev_info(chip->pdev, HW_ERR "Adjusting reported timeouts: A %lu->%luus B %lu->%luus C %lu->%luus D %lu->%luus\n", old_timeout[0], new_timeout[0], old_timeout[1], new_timeout[1], @@ -606,7 +608,7 @@ duration: chip->vendor.duration[TPM_MEDIUM] *= 1000; chip->vendor.duration[TPM_LONG] *= 1000; chip->vendor.duration_adjusted = true; - dev_info(chip->dev, "Adjusting TPM timeout parameters."); + dev_info(chip->pdev, "Adjusting TPM timeout parameters."); } return 0; } @@ -772,7 +774,7 @@ int tpm_do_selftest(struct tpm_chip *chip) * around 300ms while the self test is ongoing, keep trying * until the self test duration expires. */ if (rc == -ETIME) { - dev_info(chip->dev, HW_ERR "TPM command timed out during continue self test"); + dev_info(chip->pdev, HW_ERR "TPM command timed out during continue self test"); msleep(delay_msec); continue; } @@ -782,7 +784,7 @@ int tpm_do_selftest(struct tpm_chip *chip) rc = be32_to_cpu(cmd.header.out.return_code); if (rc == TPM_ERR_DISABLED || rc == TPM_ERR_DEACTIVATED) { - dev_info(chip->dev, + dev_info(chip->pdev, "TPM is disabled/deactivated (0x%X)\n", rc); /* TPM is disabled and/or deactivated; driver can * proceed and TPM does handle commands for @@ -930,10 +932,10 @@ int tpm_pm_suspend(struct device *dev) } if (rc) - dev_err(chip->dev, + dev_err(chip->pdev, "Error (%d) sending savestate before suspend\n", rc); else if (try > 0) - dev_warn(chip->dev, "TPM savestate took %dms\n", + dev_warn(chip->pdev, "TPM savestate took %dms\n", try * TPM_TIMEOUT_RETRY); return rc; @@ -1014,6 +1016,27 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max) } EXPORT_SYMBOL_GPL(tpm_get_random); +static int __init tpm_init(void) +{ + tpm_class = class_create(THIS_MODULE, "tpm"); + if (IS_ERR(tpm_class)) { + pr_err("couldn't create tpm class\n"); + return PTR_ERR(tpm_class); + } + + tpm_dev_init(); + return 0; +} + +static void __exit tpm_exit(void) +{ + tpm_dev_exit(); + class_destroy(tpm_class); +} + +subsys_initcall(tpm_init); +module_exit(tpm_exit); + MODULE_AUTHOR("Leendert van Doorn (leendert@watson.ibm.com)"); MODULE_DESCRIPTION("TPM Driver"); MODULE_VERSION("2.0"); diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c index 8ecb052..ee66fd4 100644 --- a/drivers/char/tpm/tpm-sysfs.c +++ b/drivers/char/tpm/tpm-sysfs.c @@ -284,16 +284,16 @@ static const struct attribute_group tpm_dev_group = { int tpm_sysfs_add_device(struct tpm_chip *chip) { int err; - err = sysfs_create_group(&chip->dev->kobj, + err = sysfs_create_group(&chip->pdev->kobj, &tpm_dev_group); if (err) - dev_err(chip->dev, + dev_err(chip->pdev, "failed to create sysfs attributes, %d\n", err); return err; } void tpm_sysfs_del_device(struct tpm_chip *chip) { - sysfs_remove_group(&chip->dev->kobj, &tpm_dev_group); + sysfs_remove_group(&chip->pdev->kobj, &tpm_dev_group); } diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 076456a..97963d3 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -28,6 +28,7 @@ #include <linux/io.h> #include <linux/tpm.h> #include <linux/acpi.h> +#include <linux/cdev.h> enum tpm_const { TPM_MINOR = 224, /* officially assigned */ @@ -153,7 +154,11 @@ enum tpm_chip_flags { }; struct tpm_chip { - struct device *dev; /* Device stuff */ + /* Device associations */ + struct device *pdev; + struct device dev; + struct cdev cdev; + const struct tpm_class_ops *ops; unsigned int flags; @@ -180,7 +185,7 @@ struct tpm_chip { static inline void tpm_chip_put(struct tpm_chip *chip) { - module_put(chip->dev->driver->owner); + module_put(chip->pdev->driver->owner); } static inline int tpm_read_index(int base, int index) @@ -376,6 +381,8 @@ struct tpm_cmd_t { tpm_cmd_params params; } __packed; +extern struct class *tpm_class; + ssize_t tpm_getcap(struct device *, __be32, cap_t *, const char *); ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, size_t bufsiz); @@ -398,6 +405,11 @@ extern void tpm_chip_unregister(struct tpm_chip *chip); int tpm_dev_add_device(struct tpm_chip *chip); void tpm_dev_del_device(struct tpm_chip *chip); +int tpm2_dev_add_device(struct tpm_chip *chip); +void tpm2_dev_del_device(struct tpm_chip *chip); +void tpm_dev_init(void); +void tpm_dev_exit(void); + int tpm_sysfs_add_device(struct tpm_chip *chip); void tpm_sysfs_del_device(struct tpm_chip *chip); diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c index fb2f921..387b0c0 100644 --- a/drivers/char/tpm/tpm_atmel.c +++ b/drivers/char/tpm/tpm_atmel.c @@ -49,7 +49,7 @@ static int tpm_atml_recv(struct tpm_chip *chip, u8 *buf, size_t count) for (i = 0; i < 6; i++) { status = ioread8(chip->vendor.iobase + 1); if ((status & ATML_STATUS_DATA_AVAIL) == 0) { - dev_err(chip->dev, "error reading header\n"); + dev_err(chip->pdev, "error reading header\n"); return -EIO; } *buf++ = ioread8(chip->vendor.iobase); @@ -60,12 +60,12 @@ static int tpm_atml_recv(struct tpm_chip *chip, u8 *buf, size_t count) size = be32_to_cpu(*native_size); if (count < size) { - dev_err(chip->dev, + dev_err(chip->pdev, "Recv size(%d) less than available space\n", size); for (; i < size; i++) { /* clear the waiting data anyway */ status = ioread8(chip->vendor.iobase + 1); if ((status & ATML_STATUS_DATA_AVAIL) == 0) { - dev_err(chip->dev, "error reading data\n"); + dev_err(chip->pdev, "error reading data\n"); return -EIO; } } @@ -76,7 +76,7 @@ static int tpm_atml_recv(struct tpm_chip *chip, u8 *buf, size_t count) for (; i < size; i++) { status = ioread8(chip->vendor.iobase + 1); if ((status & ATML_STATUS_DATA_AVAIL) == 0) { - dev_err(chip->dev, "error reading data\n"); + dev_err(chip->pdev, "error reading data\n"); return -EIO; } *buf++ = ioread8(chip->vendor.iobase); @@ -86,7 +86,7 @@ static int tpm_atml_recv(struct tpm_chip *chip, u8 *buf, size_t count) status = ioread8(chip->vendor.iobase + 1); if (status & ATML_STATUS_DATA_AVAIL) { - dev_err(chip->dev, "data available is stuck\n"); + dev_err(chip->pdev, "data available is stuck\n"); return -EIO; } @@ -97,9 +97,9 @@ static int tpm_atml_send(struct tpm_chip *chip, u8 *buf, size_t count) { int i; - dev_dbg(chip->dev, "tpm_atml_send:\n"); + dev_dbg(chip->pdev, "tpm_atml_send:\n"); for (i = 0; i < count; i++) { - dev_dbg(chip->dev, "%d 0x%x(%d)\n", i, buf[i], buf[i]); + dev_dbg(chip->pdev, "%d 0x%x(%d)\n", i, buf[i], buf[i]); iowrite8(buf[i], chip->vendor.iobase); } diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index 3b7202c..c95ddc9 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -169,7 +169,7 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len) cca = priv->cca; if (len > le32_to_cpu(cca->cmd_size)) { - dev_err(chip->dev, + dev_err(chip->pdev, "invalid command count value %x %zx\n", (unsigned int) len, (size_t) le32_to_cpu(cca->cmd_size)); @@ -200,7 +200,7 @@ static void crb_cancel(struct tpm_chip *chip) wmb(); if (crb_do_acpi_start(chip)) - dev_err(chip->dev, "ACPI Start failed\n"); + dev_err(chip->pdev, "ACPI Start failed\n"); cca->cancel = 0; } diff --git a/drivers/char/tpm/tpm_i2c_atmel.c b/drivers/char/tpm/tpm_i2c_atmel.c index 8af3b4a..dfef1ae 100644 --- a/drivers/char/tpm/tpm_i2c_atmel.c +++ b/drivers/char/tpm/tpm_i2c_atmel.c @@ -52,7 +52,7 @@ struct priv_data { static int i2c_atmel_send(struct tpm_chip *chip, u8 *buf, size_t len) { struct priv_data *priv = chip->vendor.priv; - struct i2c_client *client = to_i2c_client(chip->dev); + struct i2c_client *client = to_i2c_client(chip->pdev); s32 status; priv->len = 0; @@ -62,7 +62,7 @@ static int i2c_atmel_send(struct tpm_chip *chip, u8 *buf, size_t len) status = i2c_master_send(client, buf, len); - dev_dbg(chip->dev, + dev_dbg(chip->pdev, "%s(buf=%*ph len=%0zx) -> sts=%d\n", __func__, (int)min_t(size_t, 64, len), buf, len, status); return status; @@ -71,7 +71,7 @@ static int i2c_atmel_send(struct tpm_chip *chip, u8 *buf, size_t len) static int i2c_atmel_recv(struct tpm_chip *chip, u8 *buf, size_t count) { struct priv_data *priv = chip->vendor.priv; - struct i2c_client *client = to_i2c_client(chip->dev); + struct i2c_client *client = to_i2c_client(chip->pdev); struct tpm_output_header *hdr = (struct tpm_output_header *)priv->buffer; u32 expected_len; @@ -88,7 +88,7 @@ static int i2c_atmel_recv(struct tpm_chip *chip, u8 *buf, size_t count) return -ENOMEM; if (priv->len >= expected_len) { - dev_dbg(chip->dev, + dev_dbg(chip->pdev, "%s early(buf=%*ph count=%0zx) -> ret=%d\n", __func__, (int)min_t(size_t, 64, expected_len), buf, count, expected_len); @@ -97,7 +97,7 @@ static int i2c_atmel_recv(struct tpm_chip *chip, u8 *buf, size_t count) } rc = i2c_master_recv(client, buf, expected_len); - dev_dbg(chip->dev, + dev_dbg(chip->pdev, "%s reread(buf=%*ph count=%0zx) -> ret=%d\n", __func__, (int)min_t(size_t, 64, expected_len), buf, count, expected_len); @@ -106,13 +106,13 @@ static int i2c_atmel_recv(struct tpm_chip *chip, u8 *buf, size_t count) static void i2c_atmel_cancel(struct tpm_chip *chip) { - dev_err(chip->dev, "TPM operation cancellation was requested, but is not supported"); + dev_err(chip->pdev, "TPM operation cancellation was requested, but is not supported"); } static u8 i2c_atmel_read_status(struct tpm_chip *chip) { struct priv_data *priv = chip->vendor.priv; - struct i2c_client *client = to_i2c_client(chip->dev); + struct i2c_client *client = to_i2c_client(chip->pdev); int rc; /* The TPM fails the I2C read until it is ready, so we do the entire @@ -125,7 +125,7 @@ static u8 i2c_atmel_read_status(struct tpm_chip *chip) /* Once the TPM has completed the command the command remains readable * until another command is issued. */ rc = i2c_master_recv(client, priv->buffer, sizeof(priv->buffer)); - dev_dbg(chip->dev, + dev_dbg(chip->pdev, "%s: sts=%d", __func__, rc); if (rc <= 0) return 0; diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c index 03708e6..33c5f36 100644 --- a/drivers/char/tpm/tpm_i2c_infineon.c +++ b/drivers/char/tpm/tpm_i2c_infineon.c @@ -446,7 +446,7 @@ static int tpm_tis_i2c_recv(struct tpm_chip *chip, u8 *buf, size_t count) /* read first 10 bytes, including tag, paramsize, and result */ size = recv_data(chip, buf, TPM_HEADER_SIZE); if (size < TPM_HEADER_SIZE) { - dev_err(chip->dev, "Unable to read header\n"); + dev_err(chip->pdev, "Unable to read header\n"); goto out; } @@ -459,14 +459,14 @@ static int tpm_tis_i2c_recv(struct tpm_chip *chip, u8 *buf, size_t count) size += recv_data(chip, &buf[TPM_HEADER_SIZE], expected - TPM_HEADER_SIZE); if (size < expected) { - dev_err(chip->dev, "Unable to read remainder of result\n"); + dev_err(chip->pdev, "Unable to read remainder of result\n"); size = -ETIME; goto out; } wait_for_stat(chip, TPM_STS_VALID, chip->vendor.timeout_c, &status); if (status & TPM_STS_DATA_AVAIL) { /* retry? */ - dev_err(chip->dev, "Error left over data\n"); + dev_err(chip->pdev, "Error left over data\n"); size = -EIO; goto out; } diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c index 09f0c46..92ee9fa 100644 --- a/drivers/char/tpm/tpm_i2c_nuvoton.c +++ b/drivers/char/tpm/tpm_i2c_nuvoton.c @@ -96,13 +96,13 @@ static s32 i2c_nuvoton_write_buf(struct i2c_client *client, u8 offset, u8 size, /* read TPM_STS register */ static u8 i2c_nuvoton_read_status(struct tpm_chip *chip) { - struct i2c_client *client = to_i2c_client(chip->dev); + struct i2c_client *client = to_i2c_client(chip->pdev); s32 status; u8 data; status = i2c_nuvoton_read_buf(client, TPM_STS, 1, &data); if (status <= 0) { - dev_err(chip->dev, "%s() error return %d\n", __func__, + dev_err(chip->pdev, "%s() error return %d\n", __func__, status); data = TPM_STS_ERR_VAL; } @@ -127,13 +127,13 @@ static s32 i2c_nuvoton_write_status(struct i2c_client *client, u8 data) /* write commandReady to TPM_STS register */ static void i2c_nuvoton_ready(struct tpm_chip *chip) { - struct i2c_client *client = to_i2c_client(chip->dev); + struct i2c_client *client = to_i2c_client(chip->pdev); s32 status; /* this causes the current command to be aborted */ status = i2c_nuvoton_write_status(client, TPM_STS_COMMAND_READY); if (status < 0) - dev_err(chip->dev, + dev_err(chip->pdev, "%s() fail to write TPM_STS.commandReady\n", __func__); } @@ -212,7 +212,7 @@ static int i2c_nuvoton_wait_for_stat(struct tpm_chip *chip, u8 mask, u8 value, return 0; } while (time_before(jiffies, stop)); } - dev_err(chip->dev, "%s(%02x, %02x) -> timeout\n", __func__, mask, + dev_err(chip->pdev, "%s(%02x, %02x) -> timeout\n", __func__, mask, value); return -ETIMEDOUT; } @@ -240,7 +240,7 @@ static int i2c_nuvoton_recv_data(struct i2c_client *client, &chip->vendor.read_queue) == 0) { burst_count = i2c_nuvoton_get_burstcount(client, chip); if (burst_count < 0) { - dev_err(chip->dev, + dev_err(chip->pdev, "%s() fail to read burstCount=%d\n", __func__, burst_count); return -EIO; @@ -249,12 +249,12 @@ static int i2c_nuvoton_recv_data(struct i2c_client *client, rc = i2c_nuvoton_read_buf(client, TPM_DATA_FIFO_R, bytes2read, &buf[size]); if (rc < 0) { - dev_err(chip->dev, + dev_err(chip->pdev, "%s() fail on i2c_nuvoton_read_buf()=%d\n", __func__, rc); return -EIO; } - dev_dbg(chip->dev, "%s(%d):", __func__, bytes2read); + dev_dbg(chip->pdev, "%s(%d):", __func__, bytes2read); size += bytes2read; } @@ -264,7 +264,7 @@ static int i2c_nuvoton_recv_data(struct i2c_client *client, /* Read TPM command results */ static int i2c_nuvoton_recv(struct tpm_chip *chip, u8 *buf, size_t count) { - struct device *dev = chip->dev; + struct device *dev = chip->pdev; struct i2c_client *client = to_i2c_client(dev); s32 rc; int expected, status, burst_count, retries, size = 0; @@ -334,7 +334,7 @@ static int i2c_nuvoton_recv(struct tpm_chip *chip, u8 *buf, size_t count) break; } i2c_nuvoton_ready(chip); - dev_dbg(chip->dev, "%s() -> %d\n", __func__, size); + dev_dbg(chip->pdev, "%s() -> %d\n", __func__, size); return size; } @@ -347,7 +347,7 @@ static int i2c_nuvoton_recv(struct tpm_chip *chip, u8 *buf, size_t count) */ static int i2c_nuvoton_send(struct tpm_chip *chip, u8 *buf, size_t len) { - struct device *dev = chip->dev; + struct device *dev = chip->pdev; struct i2c_client *client = to_i2c_client(dev); u32 ordinal; size_t count = 0; diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c index b9d1a38..64ef510 100644 --- a/drivers/char/tpm/tpm_i2c_stm_st33.c +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c @@ -544,7 +544,7 @@ static int tpm_stm_i2c_recv(struct tpm_chip *chip, unsigned char *buf, size = recv_data(chip, buf, TPM_HEADER_SIZE); if (size < TPM_HEADER_SIZE) { - dev_err(chip->dev, "Unable to read header\n"); + dev_err(chip->pdev, "Unable to read header\n"); goto out; } @@ -557,7 +557,7 @@ static int tpm_stm_i2c_recv(struct tpm_chip *chip, unsigned char *buf, size += recv_data(chip, &buf[TPM_HEADER_SIZE], expected - TPM_HEADER_SIZE); if (size < expected) { - dev_err(chip->dev, "Unable to read remainder of result\n"); + dev_err(chip->pdev, "Unable to read remainder of result\n"); size = -ETIME; goto out; } @@ -671,7 +671,7 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) IRQF_TRIGGER_HIGH, "TPM SERIRQ management", chip); if (err < 0) { - dev_err(chip->dev , "TPM SERIRQ signals %d not available\n", + dev_err(chip->pdev , "TPM SERIRQ signals %d not available\n", gpio_to_irq(platform_data->io_serirq)); goto _irq_set; } diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c index dcdb671..6d49213 100644 --- a/drivers/char/tpm/tpm_infineon.c +++ b/drivers/char/tpm/tpm_infineon.c @@ -195,9 +195,9 @@ static int wait(struct tpm_chip *chip, int wait_for_bit) } if (i == TPM_MAX_TRIES) { /* timeout occurs */ if (wait_for_bit == STAT_XFE) - dev_err(chip->dev, "Timeout in wait(STAT_XFE)\n"); + dev_err(chip->pdev, "Timeout in wait(STAT_XFE)\n"); if (wait_for_bit == STAT_RDA) - dev_err(chip->dev, "Timeout in wait(STAT_RDA)\n"); + dev_err(chip->pdev, "Timeout in wait(STAT_RDA)\n"); return -EIO; } return 0; @@ -220,7 +220,7 @@ static void wait_and_send(struct tpm_chip *chip, u8 sendbyte) static void tpm_wtx(struct tpm_chip *chip) { number_of_wtx++; - dev_info(chip->dev, "Granting WTX (%02d / %02d)\n", + dev_info(chip->pdev, "Granting WTX (%02d / %02d)\n", number_of_wtx, TPM_MAX_WTX_PACKAGES); wait_and_send(chip, TPM_VL_VER); wait_and_send(chip, TPM_CTRL_WTX); @@ -231,7 +231,7 @@ static void tpm_wtx(struct tpm_chip *chip) static void tpm_wtx_abort(struct tpm_chip *chip) { - dev_info(chip->dev, "Aborting WTX\n"); + dev_info(chip->pdev, "Aborting WTX\n"); wait_and_send(chip, TPM_VL_VER); wait_and_send(chip, TPM_CTRL_WTX_ABORT); wait_and_send(chip, 0x00); @@ -257,7 +257,7 @@ recv_begin: } if (buf[0] != TPM_VL_VER) { - dev_err(chip->dev, + dev_err(chip->pdev, "Wrong transport protocol implementation!\n"); return -EIO; } @@ -272,7 +272,7 @@ recv_begin: } if ((size == 0x6D00) && (buf[1] == 0x80)) { - dev_err(chip->dev, "Error handling on vendor layer!\n"); + dev_err(chip->pdev, "Error handling on vendor layer!\n"); return -EIO; } @@ -284,7 +284,7 @@ recv_begin: } if (buf[1] == TPM_CTRL_WTX) { - dev_info(chip->dev, "WTX-package received\n"); + dev_info(chip->pdev, "WTX-package received\n"); if (number_of_wtx < TPM_MAX_WTX_PACKAGES) { tpm_wtx(chip); goto recv_begin; @@ -295,14 +295,14 @@ recv_begin: } if (buf[1] == TPM_CTRL_WTX_ABORT_ACK) { - dev_info(chip->dev, "WTX-abort acknowledged\n"); + dev_info(chip->pdev, "WTX-abort acknowledged\n"); return size; } if (buf[1] == TPM_CTRL_ERROR) { - dev_err(chip->dev, "ERROR-package received:\n"); + dev_err(chip->pdev, "ERROR-package received:\n"); if (buf[4] == TPM_INF_NAK) - dev_err(chip->dev, + dev_err(chip->pdev, "-> Negative acknowledgement" " - retransmit command!\n"); return -EIO; @@ -321,7 +321,7 @@ static int tpm_inf_send(struct tpm_chip *chip, u8 * buf, size_t count) ret = empty_fifo(chip, 1); if (ret) { - dev_err(chip->dev, "Timeout while clearing FIFO\n"); + dev_err(chip->pdev, "Timeout while clearing FIFO\n"); return -EIO; } diff --git a/drivers/char/tpm/tpm_nsc.c b/drivers/char/tpm/tpm_nsc.c index 00c5470..072c298 100644 --- a/drivers/char/tpm/tpm_nsc.c +++ b/drivers/char/tpm/tpm_nsc.c @@ -113,7 +113,7 @@ static int nsc_wait_for_ready(struct tpm_chip *chip) } while (time_before(jiffies, stop)); - dev_info(chip->dev, "wait for ready failed\n"); + dev_info(chip->pdev, "wait for ready failed\n"); return -EBUSY; } @@ -129,12 +129,12 @@ static int tpm_nsc_recv(struct tpm_chip *chip, u8 * buf, size_t count) return -EIO; if (wait_for_stat(chip, NSC_STATUS_F0, NSC_STATUS_F0, &data) < 0) { - dev_err(chip->dev, "F0 timeout\n"); + dev_err(chip->pdev, "F0 timeout\n"); return -EIO; } if ((data = inb(chip->vendor.base + NSC_DATA)) != NSC_COMMAND_NORMAL) { - dev_err(chip->dev, "not in normal mode (0x%x)\n", + dev_err(chip->pdev, "not in normal mode (0x%x)\n", data); return -EIO; } @@ -143,7 +143,7 @@ static int tpm_nsc_recv(struct tpm_chip *chip, u8 * buf, size_t count) for (p = buffer; p < &buffer[count]; p++) { if (wait_for_stat (chip, NSC_STATUS_OBF, NSC_STATUS_OBF, &data) < 0) { - dev_err(chip->dev, + dev_err(chip->pdev, "OBF timeout (while reading data)\n"); return -EIO; } @@ -154,11 +154,11 @@ static int tpm_nsc_recv(struct tpm_chip *chip, u8 * buf, size_t count) if ((data & NSC_STATUS_F0) == 0 && (wait_for_stat(chip, NSC_STATUS_F0, NSC_STATUS_F0, &data) < 0)) { - dev_err(chip->dev, "F0 not set\n"); + dev_err(chip->pdev, "F0 not set\n"); return -EIO; } if ((data = inb(chip->vendor.base + NSC_DATA)) != NSC_COMMAND_EOC) { - dev_err(chip->dev, + dev_err(chip->pdev, "expected end of command(0x%x)\n", data); return -EIO; } @@ -189,19 +189,19 @@ static int tpm_nsc_send(struct tpm_chip *chip, u8 * buf, size_t count) return -EIO; if (wait_for_stat(chip, NSC_STATUS_IBF, 0, &data) < 0) { - dev_err(chip->dev, "IBF timeout\n"); + dev_err(chip->pdev, "IBF timeout\n"); return -EIO; } outb(NSC_COMMAND_NORMAL, chip->vendor.base + NSC_COMMAND); if (wait_for_stat(chip, NSC_STATUS_IBR, NSC_STATUS_IBR, &data) < 0) { - dev_err(chip->dev, "IBR timeout\n"); + dev_err(chip->pdev, "IBR timeout\n"); return -EIO; } for (i = 0; i < count; i++) { if (wait_for_stat(chip, NSC_STATUS_IBF, 0, &data) < 0) { - dev_err(chip->dev, + dev_err(chip->pdev, "IBF timeout (while writing data)\n"); return -EIO; } @@ -209,7 +209,7 @@ static int tpm_nsc_send(struct tpm_chip *chip, u8 * buf, size_t count) } if (wait_for_stat(chip, NSC_STATUS_IBF, 0, &data) < 0) { - dev_err(chip->dev, "IBF timeout\n"); + dev_err(chip->pdev, "IBF timeout\n"); return -EIO; } outb(NSC_COMMAND_EOC, chip->vendor.base + NSC_COMMAND); diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c index 6acdb17..659de61 100644 --- a/drivers/char/tpm/tpm_ppi.c +++ b/drivers/char/tpm/tpm_ppi.c @@ -358,11 +358,11 @@ int tpm_add_ppi(struct tpm_chip *chip) ACPI_FREE(obj); - return sysfs_create_group(&chip->dev->kobj, &ppi_attr_grp); + return sysfs_create_group(&chip->pdev->kobj, &ppi_attr_grp); } void tpm_remove_ppi(struct tpm_chip *chip) { if (chip->ppi_version[0] != '\0') - sysfs_remove_group(&chip->dev->kobj, &ppi_attr_grp); + sysfs_remove_group(&chip->pdev->kobj, &ppi_attr_grp); } diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 4d4bff1..66780ba 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -244,7 +244,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count) /* read first 10 bytes, including tag, paramsize, and result */ if ((size = recv_data(chip, buf, TPM_HEADER_SIZE)) < TPM_HEADER_SIZE) { - dev_err(chip->dev, "Unable to read header\n"); + dev_err(chip->pdev, "Unable to read header\n"); goto out; } @@ -257,7 +257,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count) if ((size += recv_data(chip, &buf[TPM_HEADER_SIZE], expected - TPM_HEADER_SIZE)) < expected) { - dev_err(chip->dev, "Unable to read remainder of result\n"); + dev_err(chip->pdev, "Unable to read remainder of result\n"); size = -ETIME; goto out; } @@ -266,7 +266,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count) &chip->vendor.int_queue, false); status = tpm_tis_status(chip); if (status & TPM_STS_DATA_AVAIL) { /* retry? */ - dev_err(chip->dev, "Error left over data\n"); + dev_err(chip->pdev, "Error left over data\n"); size = -EIO; goto out; } @@ -445,7 +445,7 @@ static int probe_itpm(struct tpm_chip *chip) rc = tpm_tis_send_data(chip, cmd_getticks, len); if (rc == 0) { - dev_info(chip->dev, "Detected an iTPM.\n"); + dev_info(chip->pdev, "Detected an iTPM.\n"); rc = 1; } else rc = -EFAULT; @@ -696,7 +696,7 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle, if (devm_request_irq (dev, i, tis_int_probe, IRQF_SHARED, chip->vendor.miscdev.name, chip) != 0) { - dev_info(chip->dev, + dev_info(chip->pdev, "Unable to request irq: %d for probe\n", i); continue; @@ -746,7 +746,7 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle, if (devm_request_irq (dev, chip->vendor.irq, tis_int_handler, IRQF_SHARED, chip->vendor.miscdev.name, chip) != 0) { - dev_info(chip->dev, + dev_info(chip->pdev, "Unable to request irq: %d for use\n", chip->vendor.irq); chip->vendor.irq = 0;
Added own class for TPM devices that is used for TPM 2.0 and onwards. For TPM1 old device structure is kept for backwards compatibility. Each struct tpm_chip represents a character device that is associated to the tpm device class. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/tpm-chip.c | 14 +++++-- drivers/char/tpm/tpm-dev.c | 77 ++++++++++++++++++++++++++++++++++--- drivers/char/tpm/tpm-interface.c | 51 +++++++++++++++++------- drivers/char/tpm/tpm-sysfs.c | 6 +-- drivers/char/tpm/tpm.h | 16 +++++++- drivers/char/tpm/tpm_atmel.c | 14 +++---- drivers/char/tpm/tpm_crb.c | 4 +- drivers/char/tpm/tpm_i2c_atmel.c | 16 ++++---- drivers/char/tpm/tpm_i2c_infineon.c | 6 +-- drivers/char/tpm/tpm_i2c_nuvoton.c | 22 +++++------ drivers/char/tpm/tpm_i2c_stm_st33.c | 6 +-- drivers/char/tpm/tpm_infineon.c | 22 +++++------ drivers/char/tpm/tpm_nsc.c | 20 +++++----- drivers/char/tpm/tpm_ppi.c | 4 +- drivers/char/tpm/tpm_tis.c | 12 +++--- 15 files changed, 199 insertions(+), 91 deletions(-)