lkml.org 
[lkml]   [2019]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 1/1] drivers: misc: Add support for nCipher HSM devices
Date
Hi Greg,

I'm just about to push v2 of our patch but wanted to address your comments in the emails you raised them in.

> > +
> > +/* Device ioctl struct definitions */
> > +
> > +/* Result of the ENQUIRY ioctl. */
> > +struct nfdev_enquiry_str {
> > + __u32 busno; /**< Which bus is the PCI device on. */
> > + __u8 slotno; /**< Which slot is the PCI device in. */
> > + __u8 reserved[3]; /**< for consistent struct alignment */
>
> What is this crazy /**< text?
>
> Please use correct kerneldoc formatting to help document things.

Done. I've also double checked all the other files. Checkpatch didn't seem to complain about these so we missed them, sorry.

> > + */
> > +#define NFDEV_CONTROL_HARMLESS(c) ((c) <= 1)
>
> Why does userspace need this?
>

It doesn't. We've removed that and a couple other leftovers.

> > +enum {
> > + /** Enquiry ioctl.
> > + * \return nfdev_enquiry_str describing the attached device.
> > + */
> > + NFDEV_IOCTL_NUM_ENQUIRY = 1,
> > +
> > + /** Channel Update ioctl.
> > + * \deprecated
> > + */
> > + NFDEV_IOCTL_NUM_CHUPDATE,
>
> You have to explicitly set your enums if you want them to work properly in an
> ioctl. As it is, this will fail in nasty ways you can never debug :(
>

Thanks, we've set them explicitly and also removed the unnecessary ones.

> > +
> > +#define NFDEV_IOCTL_DEBUG \
> > + _IOW(NFDEV_IOCTL_TYPE, NFDEV_IOCTL_NUM_DEBUG, int)
>
> Why do you care about debugging to userspace through an ioctl? Just use
> debugfs and be done with it, that's what it is there for. Also you can use
> dynamic debugging (hopefully you already are) and use that kernel-wide
> api/interface.
>
> Individual drivers should NEVER have custom debugging controls and macros.

Yes, this was accidentally left in. It's been removed now.

>
> > +
> > +#define NFDEV_IOCTL_PCI_IFVERS \
> > + _IOW(NFDEV_IOCTL_TYPE, NFDEV_IOCTL_NUM_PCI_IFVERS, int)
>
> Can't you get this from the pci device information?
>

Unfortunately not. Each time the device is brought up or has its state changed, the IFVERS is re-negotiated. This call is our means of discovering what IFVER is supported by the currently running firmware.

Thanks,
Dave

\
 
 \ /
  Last update: 2019-12-20 16:29    [W:0.054 / U:19.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site