On Wed, 2015-04-08 at 10:28 -0600, Jason Gunthorpe wrote: > On Wed, Apr 08, 2015 at 10:26:07AM +0300, Jarkko Sakkinen wrote: > > On Wed, Apr 01, 2015 at 12:19:25PM -0600, Jason Gunthorpe wrote: > > > On Wed, Apr 01, 2015 at 03:28:52PM +0300, Jarkko Sakkinen wrote: > > > > Added PPI interface to the character device. PPI interface is also kept > > > > in the pdev for backwards compatibility. > > > > > > Could you look at just completely moving the PPI interface to the char > > > dev and then adding a symlink from the pdev? That would be really > > > ideal. > > > > > > symlinks have the advantage that they actually fully fix the lifetime > > > issues. > > > > > > This seems doable, if we replace the ppi_attrs group with a bunch of > > > calls to sysfs_create_link it should work ? > > > > If we follow the pattern in [1] by the book, how would you use > > sysfs_create_link()? To be more specific, how would you get the driver > > core to create the symlinks for you? > > The driver core does not create symlinks, it creates the real files, > which is the tpm_class->dev_groups part of your patch. That is fine.. > > The symlinks replace the broken legacy files under the > platform_device. These are already racy, and different versions of the > kernel have had different kind of races here. It wasn't until your > 'tpm: fix call order in tpm-chip.c' that the ordering here started to > make any kind of sense. > > So, I'm inclined to say the legacy paths don't much matter. They have > rarely worked race free so nothing out there can be depending on > them. > > I'd rather see the legacy paths be turned into symlinkes because that > means we can close the use-after-free oops possibility the current > code has, and that is a more serious bug than the user space race > which has always existed anyhow.
OK, I'll consider doing this for the next iteration. > > If we decide not to follow [1] by the book, then this might be doable > > (thinking off my head, that's the reason why I use *might be* instead > > of *is*). Wouldn't we get non-racy behavior if sysfs_create_link()'s > > are executed after device_initialize() and before device_add()? > > That would at least preserve the latest semantic that the uevent is > created after all the sysfs is in place. It is the best we can > do. > > Since this seems to address the race problem why do you think this is > not worthwhile? > > > > > + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) > > > > + return -EINVAL; > > > > > > Hum, I don't think the PPI files should be created if there is no PPI > > > support.. > > > > Again, following [1] by the book. And again, I think we could just as > > well do our sysfs stuff in-between device_initialize() and device_add() > > and get the non-racy behavior. > > Not relying on the class default seems reasonable for ppi to me. > > > I do not think it would be a bad idea to always create them when the > > kernel is compiled with CONFIG_ACPI. Maybe it would be abetter idea to > > return -ENOSYS? > > This is not really consistent with other uses of sysfs, user space > tooling has a harder time detecting ENOSYS than it does a file that > doesn't exist. > > It is also a change from the current PPI behavior, so I don't think we > should do this without a very good reason. Agreed. > > Device Model in the Linux kernel seems to recommend > > through the defaults APIs a flat set of attributes for each device > > node. > > No, that is just the defaults scheme, there are other ways to > create attributes that are conditional based on device capabilities. > > Greg notes: > > > Sometimes you don't have control over the driver either, or want > > different sysfs files for different devices controlled by your driver > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > drivers/firewire/core-device.c has an example of this, the > config_rom_attributes are pruned to only expose the ones that actually > exist using the struct device groups scheme. Thanks for noting this. > Jason OK based on this discussion I'll iterate the patch. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
