Re: [PATCH v2 04/12] mini-os: use alloc_file_type() and get_file_from_fd() in tpm_tis

[Andrew Cooper <amc96@xxxxxxxx>]

On 12/01/2022 07:54, Juergen Gross wrote:
> On 11.01.22 21:29, Andrew Cooper wrote:
>> On 11/01/2022 15:12, Juergen Gross wrote:
>>> diff --git a/tpm_tis.c b/tpm_tis.c
>>> index 477f555..abea7a1 100644
>>> --- a/tpm_tis.c
>>> +++ b/tpm_tis.c
>>> @@ -1093,6 +1097,23 @@ ssize_t tpm_getcap(struct tpm_chip *chip,
>>> uint32_t subcap_id, cap_t *cap,
>>>           return rc;
>>>   }
>>>   +static void shutdown_tpm_tis(struct tpm_chip* tpm){
>>
>> Style, as you're moving it.  Also in the function.
>>
>>> @@ -1360,6 +1369,35 @@ int tpm_tis_posix_fstat(int fd, struct stat*
>>> buf)
>>>      return 0;
>>>   }
>>>   +static struct file_ops tpm_tis_ops = {
>>> +    .name = "tpm_tis",
>>> +    .read = tpm_tis_posix_read,
>>> +    .write = tpm_tis_posix_write,
>>> +    .lseek = lseek_default,
>>> +    .close = tpm_tis_close,
>>> +    .fstat = tpm_tis_posix_fstat,
>>> +};
>>> +
>>> +int tpm_tis_open(struct tpm_chip* tpm)
>>
>> Style.
>
> Ah, yes I should have corrected this while moving the function.
>
>>
>>> +{
>>> +   struct file *file;
>>> +   static unsigned int ftype_tis;
>>> +
>>> +   /* Silently prevent multiple opens */
>>> +   if(tpm->fd != -1) {
>>> +      return tpm->fd;
>>> +   }
>>
>> Another WTF moment.  We silently swallow multiple open()s, but don't
>> refcout close()s ?
>>
>> This cannot be correct, or sensible, behaviour.
>>
>> Jason/Daniel - thoughts?
>
> I just moved the function, but I can change this, of course.

It's a pattern elsewhere too.  Much as it's horrible logic, it probably
doesn't want reworking in a patch like this, and the series as a whole
is big enough so I'm not going to suggest you tackle it too.

~Andrew