| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4E60EC61.4030004@linux.vnet.ibm.com>
Date: Fri, 02 Sep 2011 11:46:57 -0300
From: Rajiv Andrade <srajiv@...ux.vnet.ibm.com>
To: Christophe Henri RICARD <christophe-h.ricard@...com>
CC: Mohamed TABET <mohamed.tabet@...com>,
Marcel Selhorst <m.selhorst@...rix.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
James Morris <jmorris@...ei.org>,
"joe@...ches.com" <joe@...ches.com>,
matt mooney <mfm@...eddisk.com>,
Sean NEWTON <sean.newton@...com>,
Jean-Luc BLANC <jean-luc.blanc@...com>
Subject: Re: [PATCH 1/1] TPM: new stm i2c device driver
On 25-08-2011 16:05, Christophe Henri RICARD wrote:
> The 1/3 was placed by accident. I did reply on this one to make sure everybody would be able to do the follow up.
> Just did the correction.
The ideal would be you submitting another one, that applies on top of
James' security-testing-2.6 tree.
Also, make sure you run scripts/checkpatch.pl over it before
submitting, its result so far:
total: 991 errors, 287 warnings, 882 lines checked
The code doesn't have a single tab, only spaces. Not sure if it was due
a mail client configuration, if so take a look at Documentation/email-clients.txt,
if that wasn't the case, make sure you run scripts/Lindent over you code prior to
posting it.
What does st9 in the driver's name stand for? Please rename it to tpm_stm_i2c.
More comments below:
> Thanks
> Christophe
>>>> +
>>>> +#define TPM_STS_DATA_AVAIL 0x10
This one and..
>> TPM_STS_ACCEPT_COMMAND)
>>>> +enum tis_defaults {
>>>> + TIS_SHORT_TIMEOUT = 750, /* ms */
>>>> + TIS_LONG_TIMEOUT = 2000, /* 2 sec */
>>>> +};
>>>> +
... these values are in the tpm_tis.c already, given there's now more drivers using it, they
should be moved to tpm.h then and not get defined again.
>>>> +/*
>>>> + * gpio_readpin is a wrapper to read a gpio value.
>>>> + * Use generic gpio APIs
>>>> + * @param: pin_id, the pin identifier where the value will be read.
>>>> + * @return: the gpio value (should be 0 or 1) or negative errno
>>>> + */
>>>> +static int gpio_readpin(int pin_id)
>>>> +{
>>>> + int ret;
>>>> + ret = gpio_direction_input(pin_id);
>>>> + if (ret == 0)
>>>> + return gpio_get_value(pin_id);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +/*
>>>> + * tpm_stm_i2c_status is not implemented because TIS registers are not implemented.
>>>> + */
>>>> +static u8 tpm_stm_i2c_status(struct tpm_chip *chip)
>>>> +{
>>>> + u8 state_data, state_command;
>>>> +
>>>> + state_data = gpio_readpin(pin_infos->data_avail_pin);
>>>> + state_command = gpio_readpin(pin_infos->accept_pin);
>>>> + return (state_data << 4) | state_command;
>>>> +}
>>>> +
>>>> +static int wait_for_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
>>>> + wait_queue_head_t *queue)
Sounds a copy & paste issue here, but *queue isn't being used here.
>>>> +{
>>>> + unsigned long stop;
>>>> + u8 status;
>>>> +
>>>> + FUNC_ENTER();
>>>> +
>>>> + /* check current status */
>>>> + status = tpm_stm_i2c_status(chip);
>>>> + if (status == mask)
>>>> + return 0;
>>>> + if (status == TPM_STS_CANCEL)
>>>> + goto end;
>>>> + stop = jiffies + timeout;
>>>> + do {
>>>> + msleep(TPM_TIMEOUT);
>>>> + status = tpm_stm_i2c_status(chip);
>>>> + if ((status & mask) == mask)
>>>> + return 0;
>>>> + } while (time_before(jiffies, stop));
>>>> +end:
>>>> + return -ETIME;
>>>> +}
>>>> +
This is very similar to what's in tpm_tis.c, given there's one more user for it now, it's a good time
to move it to tpm.c and provide it tpm_stm_i2c_status or tpm_tis_status as a function pointer.
>>>> +/*
>>>> + * tpm_st19_i2c_ioctl provides 2 handles:
>>>> + * - TPMIOC_CANCEL: allow to CANCEL a TPM commands execution.
>>>> + * See tpm_stm_i2c_cancel description above
>>>> + * - TPMIOC_TRANSMIT: allow to transmit a TPM commands.
>>>> + *
>>>> + * @return: In case of success, return TPM response size.
>>>> + * In other case return < 0 value describing the issue.
>>>> + */
>>>> +/*#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 36)
>>>> +static ssize_t tpm_st19_i2c_ioctl(struct inode *inode, struct file
>>>> *file,
>>>> + unsigned int cmd, unsigned long arg)
>>>> +#else*/
>>>> +static long tpm_st19_i2c_unlocked_ioctl(struct file *file,
>>>> + unsigned int cmd, unsigned long arg)
Please remove the commented ifdef logic.
>>>> +/*#endif*/
>>>> +{
>>>> + int in_size = 0, out_size = 0, ret = -ENOTTY;
>>>> + struct tpm_chip *chip = file->private_data;
>>>> +
>>>> + lock_kernel();
>>>> + switch (cmd) {
>>>> + case TPMIOC_CANCEL:
>>>> + tpm_stm_i2c_cancel(chip);
>>>> + ret = -ENOSYS;
>>>> + break;
>>>> + case TPMIOC_TRANSMIT:
>>>> + if (copy_from_user(chip->data_buffer,
>>>> + (const char *)arg,
>> TPM_HEADER_SIZE)) {
>>>> + ret = -EFAULT;
>>>> + break;
>>>> + }
>>>> +
>>>> + in_size = be32_to_cpu(*(__be32 *) (chip->data_buffer +
>> 2));
>>>> + if (copy_from_user(chip->data_buffer,
>>>> + (const char *)arg, in_size)) {
>>>> + ret = -EFAULT;
>>>> + break;
>>>> + }
>>>> +
>>>> + tpm_stm_i2c_send(chip, chip->data_buffer, in_size);
>>>> +
>>>> + out_size = tpm_stm_i2c_recv(chip, chip->data_buffer,
>>>> + TPM_BUFSIZE);
>>>> + if (copy_to_user((char *)arg, chip->data_buffer,
>> out_size))
>>>> {
>>>> + ret = -EFAULT;
>>>> + break;
>>>> + }
>>>> + ret = out_size;
>>>> + break;
>>>> + }
>>>> + unlock_kernel();
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct file_operations tpm_st19_i2c_fops = {
>>>> + .owner = THIS_MODULE,
>>>> + .llseek = no_llseek,
>>>> + .read = tpm_read,
>>>> +
>>>> + /*#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 36)
>>>> + .ioctl = tpm_st19_i2c_ioctl,
>>>> + #else */
>>>> + .unlocked_ioctl = tpm_st19_i2c_unlocked_ioctl,
>>>> + /*#endif */
Same here.
Rajiv
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists