> -----Original Message-----
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Friday, October 20, 2017 5:54 PM
> To: Oleksandr Shamray <oleksan...@mellanox.com>
> Cc: a...@arndb.de; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org;
> open...@lists.ozlabs.org; j...@jms.id.au; j...@resnulli.us;
> tklau...@distanz.ch; linux-ser...@vger.kernel.org; m...@shout.net; Vadim
> Pasternak <vad...@mellanox.com>; system-sw-low-level <system-sw-low-
> le...@mellanox.com>; robh...@kernel.org; openocd-devel-
> ow...@lists.sourceforge.net; linux-...@vger.kernel.org;
> da...@davemloft.net; mche...@kernel.org; Jiri Pirko <j...@mellanox.com>
> Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver
>
> On Fri, Oct 20, 2017 at 02:34:00PM +0000, Oleksandr Shamray wrote:
> > Hi Greg.
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gre...@linuxfoundation.org]
> > > Sent: Friday, October 20, 2017 2:55 PM
> > > To: Oleksandr Shamray <oleksan...@mellanox.com>
> > > Cc: a...@arndb.de; linux-kernel@vger.kernel.org; linux-arm-
> > > ker...@lists.infradead.org; devicet...@vger.kernel.org;
> > > open...@lists.ozlabs.org; j...@jms.id.au; j...@resnulli.us;
> > > tklau...@distanz.ch; linux-ser...@vger.kernel.org; m...@shout.net;
> > > Vadim Pasternak <vad...@mellanox.com>; system-sw-low-level
> > > <system-sw-low- le...@mellanox.com>; robh...@kernel.org;
> > > openocd-devel- ow...@lists.sourceforge.net;
> > > linux-...@vger.kernel.org; da...@davemloft.net; mche...@kernel.org;
> > > Jiri Pirko <j...@mellanox.com>
> > > Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver
> > >
> > > On Thu, Sep 21, 2017 at 12:25:29PM +0300, Oleksandr Shamray wrote:
> > > > +struct jtag {
> > > > + struct device *dev;
> > > > + struct cdev cdev;
> > >
> > > Why are you using a cdev here and not just a normal misc device?
> >
> > What the benefits to use misc instead of cdev?
>
> Less code, simpler logic, easier to review and understand, etc.
>
> Let me ask you, why use a cdev instead of a misc?
As I know misc device more applicable if we want to create one device f.e.
/dev/jtag.
But in current case we can have more than one jtag device /dev/jtag0 ...
/dev/jtagN.
So I decided to use cdev.
>
> > > I forgot if this is what you were doing before, sorry...
> > >
> > > > + int id;
> > > > + atomic_t open;
> > >
> > > Why do you need this?
> >
> > This counter used to avoid open at the same time by 2 or more users.
>
> But it isn't working :)
>
> And why do you care?
>
> > > > + const struct jtag_ops *ops;
> > > > + unsigned long priv[0] __aligned(ARCH_DMA_MINALIGN);
> > >
> > > Huh? Why is this needed to be dma aligned? Why not just use the
> > > private pointer in struct device?
> > >
> >
> > It is critical?
>
> You are saying it is, so you have to justify it. There is a pointer for you
> to use,
> don't make new ones for no reason, right?
>
You are right. Will remove.
> > > > +};
> > > > +
> > > > +static dev_t jtag_devt;
> > > > +static DEFINE_IDA(jtag_ida);
> > > > +
> > > > +void *jtag_priv(struct jtag *jtag) {
> > > > + return jtag->priv;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(jtag_priv);
> > > > +
> > > > +static u8 *jtag_copy_from_user(__u64 udata, unsigned long bit_size) {
> > > > + unsigned long size;
> > > > + void *kdata;
> > > > +
> > > > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > > > + kdata = memdup_user(u64_to_user_ptr(udata), size);
> > >
> > > You only use this once, why not just open-code it?
> >
> > I think it makes code more understandable.
>
> As a reviewer, I don't :)
Ok, I will fix :)
>
> > > > +
> > > > + return kdata;
> > > > +}
> > > > +
> > > > +static unsigned long jtag_copy_to_user(__u64 udata, u8 *kdata,
> > > > + unsigned long bit_size) {
> > > > + unsigned long size;
> > > > +
> > > > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > > > +
> > > > + return copy_to_user(u64_to_user_ptr(udata), (void *)(kdata),
> > > > +size);
> > >
> > > Same here, making this a separate function seems odd.
> >
> > Same, I think it makes code more understandable.
>
> But it doesn't.
>
Ok, I will fix :)
> > > > +
> > > > + if (jtag->ops->freq_set)
> > > > + err = jtag->ops->freq_set(jtag, value);
> > > > + else
> > > > + err = -EOPNOTSUPP;
> > > > + break;
> > > > +
> > > > + case JTAG_IOCRUNTEST:
> > > > + if (copy_from_user(&idle, (void *)arg,
> > > > + sizeof(struct jtag_run_test_idle)))
> > > > + return -ENOMEM;
> > > > + err = jtag_run_test_idle_op(jtag, &idle);
> > >
> > > Who validates the structure fields? Is that up to the individual
> > > jtag driver? Why not do it in the core correctly so that it only
> > > has to be done in one place and you do not have to audit every individual
> driver?
> >
> > Input parameters validated by jtag platform driver. I think it not
> > critical.
>
> Not true at all. It is very critical. Remmeber, "All Input Is Evil!"
>
> You have to validate this. I as a reviewer have to find where you are
> validating
> this data to ensure bad things do not happen. I can't review that here, now I
> have to go and review all of the individual drivers, which is a major pain,
> don't
> you agree?
Agree. I will add input parameter checking here before call device driver.
>
> > > > + break;
> > > > +
> > > > + case JTAG_IOCXFER:
> > > > + if (copy_from_user(&xfer, (void *)arg,
> > > > + sizeof(struct jtag_xfer)))
> > > > + return -EFAULT;
> > > > +
> > > > + if (xfer.length >= JTAG_MAX_XFER_DATA_LEN)
> > > > + return -EFAULT;
> > > > +
> > > > + xfer_data = jtag_copy_from_user(xfer.tdio, xfer.length);
> > > > + if (!xfer_data)
> > > > + return -ENOMEM;
> > >
> > > Are you sure that's the correct error value?
> >
> > I think yes, but what you suggest?
>
> A fault happened, so -EFAULT, right?
>
Right.
> > [..]
> > > + .unlocked_ioctl = jtag_ioctl,
> > > + .open = jtag_open,
> > > + .release = jtag_release,
> > > +};
> >
> > add a compat_ioctl pointer here, after ensuring that all ioctl
> > commands are compatible between 32-bit and 64-bit user space.
> > [..]
>
> And if you do not, what happens? You shouldn't need it as there is no fixups
> necessary, or am I mistaken about that?
Yes, you are right. In code compat_ioctl called same function as in
unlocked_ioctl.
So I can remove compat and system will always call unlocked_ioctl.
>
> > > > +static int jtag_open(struct inode *inode, struct file *file) {
> > > > + struct jtag *jtag = container_of(inode->i_cdev, struct jtag,
> > > > +cdev);
> > > > +
> > > > + if (atomic_read(&jtag->open)) {
> > > > + dev_info(NULL, "jtag already opened\n");
> > > > + return -EBUSY;
> > >
> > > Why do you care if multiple opens can happen?
> >
> > Jtag HW not support to using with multiple requests from different users. So
> we prohibit this.
>
> Why does the kernel care?
>
> And again, your implementation is broken, it's not actually doing this
> protection. I recommend just not doing it at all, but if you really are
> insisting
> on it, you have to get it correct :)
I will follow your recommendations and remove it.
>
> thanks,
>
> greg k-h
Thanks.
Oleksandr S
