Re: [PATCH v10 03/12] peci: Add support for PECI bus driver core

[Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>]

Hi Greg,

On 1/23/2019 10:57 PM, Greg Kroah-Hartman wrote:
> On Wed, Jan 23, 2019 at 01:38:24PM -0800, Jae Hyun Yoo wrote:
>> Hi Greg,
>>
>> Thanks for sharing your time on reviewing this patch. Please find my
>> answers below.
>>
>> On 1/22/2019 5:20 AM, Greg Kroah-Hartman wrote:
>>> On Mon, Jan 07, 2019 at 01:41:27PM -0800, Jae Hyun Yoo wrote:
>>>> +config PECI
>>>> +	bool "PECI support"
>>>> +	select RT_MUTEXES
>>>> +	select CRC8
>>>> +	help
>>>> +	  The Platform Environment Control Interface (PECI) is a one-wire bus
>>>> +	  interface that provides a communication channel from Intel processors
>>>> +	  and chipset components to external monitoring or control devices.
>>>
>>> Why can't this be built as a module?
>>
>> This PECI core driver should be prepared ahead of any PECI adapter
>> driver registration which can be called from 'kernel_init' if the
>> adapter driver is built-in. So this driver uses 'postcore_initcall' and
>> it's the reason why it can't be built as a module.
> 
> Then set up your dependancies correctly.  As an example, you can have
> the USB core as a module, and you are not allowed to have USB drivers
> built into the kernel.  It's not difficult to do this.  Please make your
> core also be a module so that you do not burden the zillions of machines
> out there with code that they do not need, just because you forced the
> distro to choose "Y" or "N".

Okay. I'll make this core config as a tristate.

>>>> +	u8 *msg;
>>>> +
>>>> +	if (!capable(CAP_SYS_ADMIN))
>>>> +		return -EPERM;
>>>
>>> Really?  Nice, you have userspace tools running as root, what could go
>>> wrong... :)
>>
>> I didn't catch your point. Should it be removed?
> 
> You are forcing your userspace tools to have CAP_SYS_ADMIN in order to
> talk to your device.  Why?  What is wrong with the file permissions on
> the device node instead?  Why can't userspace decide what user should be
> able to talk to your device?
> 
> Don't require special permissions for no good reason.  This forces
> whatever userspace tool that handles this, to have enough permission to
> do anything it wants in the system.  And I do not think you want that.

Okay, I got your point now. Thank you. I'll remove this permission check
from here.

>>>> +static int peci_detect(struct peci_adapter *adapter, u8 addr)
>>>> +{
>>>> +	struct peci_ping_msg msg;
>>>> +
>>>> +	msg.addr = addr;
>>>
>>> What about the un-initialized fields in this structure?  Can you
>>> properly handle that, and also, is this ok to be on the stack?
>>
>> It's fully initialized at here because the peci_ping_msg struct has only
>> one member:
>>
>> struct peci_ping_msg {
>> 	__u8 addr;
>> };
> 
> Ok.  But my question about "can you do this off the stack" remains.

I'll add 3 bytes of dummy padding into this structure. Also, I'll check
again u32 boundary alignment for all struct defines in peci_ioctl.h.
Would it be okay to be on stack then?

>>>> +static ssize_t peci_sysfs_new_device(struct device *dev,
>>>> +				     struct device_attribute *attr,
>>>> +				     const char *buf, size_t count)
>>>> +{
>>>> +	struct peci_adapter *adapter = to_peci_adapter(dev);
>>>> +	struct peci_board_info info = {};
>>>> +	struct peci_client *client;
>>>> +	char *blank, end;
>>>> +	int rc;
>>>> +
>>>> +	/* Parse device type */
>>>> +	blank = strchr(buf, ' ');
>>>> +	if (!blank) {
>>>> +		dev_err(dev, "%s: Missing parameters\n", "new_device");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +	if (blank - buf > PECI_NAME_SIZE - 1) {
>>>> +		dev_err(dev, "%s: Invalid device type\n", "new_device");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +	memcpy(info.type, buf, blank - buf);
>>>> +
>>>> +	/* Parse remaining parameters, reject extra parameters */
>>>> +	rc = sscanf(++blank, "%hi%c", &info.addr, &end);
>>>
>>> Please do not tell me you are parsing a sysfs write to do some type of
>>> new configuration.  That is not what sysfs is for, that is what configfs
>>> is for, please use that instead, this isn't ok.
>>
>> This is for run-time registration of a PECI client which is connected to
>> a PECI bus adapter. The life cycle of this sysfs interface will be
>> synced with the adapter driver. Actually, it follows what I2C core
>> driver currently does.
> 
> What i2c core driver parses configuration options in sysfs?
> 
> Ugh, I see that now.  That's horrible.  Please do not emulate that at
> all.
> 
> Again, use configfs, that is what it is there for, do not spread bad
> interfaces to new places in the kernel.

Okay, I see. I'll rewrite this interface using configfs. Thanks!

>>>> +	if (rc < 1) {
>>>> +		dev_err(dev, "%s: Can't parse client address\n", "new_device");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +	if (rc > 1  && end != '\n') {
>>>> +		dev_err(dev, "%s: Extra parameters\n", "new_device");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	client = peci_new_device(adapter, &info);
>>>> +	if (!client)
>>>> +		return -EINVAL;
>>>> +
>>>> +	/* Keep track of the added device */
>>>> +	mutex_lock(&adapter->userspace_clients_lock);
>>>> +	list_add_tail(&client->detected, &adapter->userspace_clients);
>>>> +	mutex_unlock(&adapter->userspace_clients_lock);
>>>> +	dev_info(dev, "%s: Instantiated device %s at 0x%02hx\n", "new_device",
>>>> +		 info.type, info.addr);
>>>
>>> Don't be noisy for things that are expected to happen.
>>
>> I think it should print out a result on a userspace request for client
>> module registration.
> 
> Why?  Who is going to do anything with this?  Userspace "knows" it
> worked because their function call returned success.  It is not going to
> then parse a kernel log.

Agree. Will change it to dev_dbg().

>>>> +
>>>> +	return count;
>>>> +}
>>>> +static DEVICE_ATTR(new_device, 0200, NULL, peci_sysfs_new_device);
>>>
>>> Not that you should be doing this, but DEVICE_ATTR_RO() is what you want
>>> here, right?
>>
>> Actually, DEVICE_ATTR_WO() is what I can use here. The reason why I used
>> DEVICE_ATTR() is for keeping the function naming pattern as
>> 'peci_sysfs_new_device()' instead of 'new_device_store()'.
> 
> Sorry, yes, WO is what you want.  But anyway, this should be in
> configfs, not sysfs.

I'll rewrite this interface using configfs.

>>>> +static ssize_t peci_sysfs_delete_device(struct device *dev,
>>>> +					struct device_attribute *attr,
>>>> +					const char *buf, size_t count)
>>>> +{
>>>> +	struct peci_adapter *adapter = to_peci_adapter(dev);
>>>> +	struct peci_client *client, *next;
>>>> +	struct peci_board_info info = {};
>>>> +	struct peci_driver *driver;
>>>> +	char *blank, end;
>>>> +	int rc;
>>>> +
>>>> +	/* Parse device type */
>>>> +	blank = strchr(buf, ' ');
>>>> +	if (!blank) {
>>>> +		dev_err(dev, "%s: Missing parameters\n", "delete_device");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +	if (blank - buf > PECI_NAME_SIZE - 1) {
>>>> +		dev_err(dev, "%s: Invalid device type\n", "delete_device");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +	memcpy(info.type, buf, blank - buf);
>>>> +
>>>> +	/* Parse remaining parameters, reject extra parameters */
>>>> +	rc = sscanf(++blank, "%hi%c", &info.addr, &end);
>>>> +	if (rc < 1) {
>>>> +		dev_err(dev, "%s: Can't parse client address\n",
>>>> +			"delete_device");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +	if (rc > 1  && end != '\n') {
>>>> +		dev_err(dev, "%s: Extra parameters\n", "delete_device");
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>> Same here, no parsing of configurations through sysfs, that is not ok.
>>> Again, use configfs.
>>
>> Same as above. This is for run-time deregistration of a PECI client
>> which is registered on a PECI bus adapter. The life cycle of this sysfs
>> interface will be synced with the adapter driver. Actually, it follows
>> what I2C core driver currently does.
> 
> Again, do not copy previous mistakes please.

Agreed. Thanks!

>>>> +
>>>> +	return rc;
>>>> +}
>>>> +static DEVICE_ATTR_IGNORE_LOCKDEP(delete_device, 0200, NULL,
>>>> +				  peci_sysfs_delete_device);
>>>
>>> sysfs files that remove themselves are reserved for a specific circle in
>>> hell, you really don't want to mess with them :(
>>
>> It cannot remove itself. The owner of the sysfs files is an adapter
>> driver and only client devices can be added/removed by these sysfs
>> files. An adapter can't be removed by accessing of this sysfs.
> 
> They why the LOCKDEP notation?

For a case of debugging. Anyway, I'll rewrite this part using configfs.

>>>> +/**
>>>> + * struct peci_adapter - represent a PECI adapter
>>>> + * @owner: owner module of the PECI adpater
>>>> + * @bus_lock: mutex for exclusion of multiple callers
>>>> + * @dev: device interface to this driver
>>>> + * @cdev: character device object to create character device
>>>> + * @nr: the bus number to map
>>>> + * @name: name of the adapter
>>>> + * @userspace_clients_lock: mutex for exclusion of clients handling
>>>> + * @userspace_clients: list of registered clients
>>>> + * @xfer: low-level transfer function pointer of the adapter
>>>> + * @cmd_mask: mask for supportable PECI commands
>>>> + *
>>>> + * Each PECI adapter can communicate with one or more PECI client children.
>>>> + * These make a small bus, sharing a single wired PECI connection.
>>>> + */
>>>> +struct peci_adapter {
>>>> +	struct module		*owner;
>>>> +	struct rt_mutex		bus_lock;
>>>> +	struct device		dev;
>>>> +	struct cdev		cdev;
>>>
>>> Yeah, two differently reference counted variables in the same structure,
>>> what could go wrong!  :(
>>>
>>> Don't do this, make your cdev a pointer to be sure you get things right,
>>> otherwise this will never work properly.
>>>
>>> And shouldn't the character device be a class device?  Don't confuse
>>> devices in the driver model with the interaction of them to userspace in
>>> a specific manner, those should be separate things, right?
>>
>> Okay, I got your point. I'll split out the character device part as
>> a separated module and will make the module can be attached to a PECI
>> bus.
> 
> I'm not saying it has to be a whole separate module, at the least it
> needs to be a new structure.  As it is, your code is not correct due to
> the dual-structures-trying-to-fight-it-out-for-lifecycle-rules

I got you point clearly now. I'll use a separate structure for the
character device.

>>>> +/**
>>>> + * struct peci_xfer_msg - raw PECI transfer command
>>>> + * @addr; address of the client
>>>> + * @tx_len: number of data to be written in bytes
>>>> + * @rx_len: number of data to be read in bytes
>>>> + * @tx_buf: data to be written, or NULL
>>>> + * @rx_buf: data to be read, or NULL
>>>> + *
>>>> + * raw PECI transfer
>>>> + */
>>>> +struct peci_xfer_msg {
>>>> +	__u8 addr;
>>>> +	__u8 tx_len;
>>>> +	__u8 rx_len;
>>>> +	__u8 tx_buf[PECI_BUFFER_SIZE];
>>>> +	__u8 rx_buf[PECI_BUFFER_SIZE];
>>>> +} __attribute__((__packed__));
>>>
>>> Go kick the hardware engineer who did not align things on a 32bit
>>> boundry.  They should have known better :(
>>
>> I intended to make it as contiguous byte sequence in this order because
>> some PECI commands need to caclulate CRC8 checksum using this as a byte
>> array. I'll align these on a 32bit boundary and will use a temporary
>> buffer instead while calcuating a CRC8 checksum.
> 
> If this really is the way the structure needs to be on the wire, that's
> fine, it's just a complaint about the protocol and how bad accessing
> those fields are going to suck for some processors.
> 
> But if you can get away with not doing it like this, that would be nice.

OK. I'll align this on a 32bit boundary and align all remaining
structures in this header file as well.

Thanks a lot for your comments.

Regards,
Jae