RE: [patch v21 1/4] drivers: jtag: Add JTAG core driver

[Oleksandr Shamray]

Hi Andy. 
Thanks for review.

Please read my answers inline.

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@xxxxxxxxx]
> Sent: 16 ÐÐÑ 2018 Ð. 0:22
> To: Oleksandr Shamray <oleksandrs@xxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Arnd Bergmann 
> <arnd@xxxxxxxx>; Linux Kernel Mailing List 
> <linux-kernel@xxxxxxxxxxxxxxx>; linux-arm Mailing List 
> <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; devicetree 
> <devicetree@xxxxxxxxxxxxxxx>; openbmc@xxxxxxxxxxxxxxxx; Joel Stanley 
> <joel@xxxxxxxxx>; JiÅÃ PÃrko <jiri@xxxxxxxxxxx>; Tobias Klauser 
> <tklauser@xxxxxxxxxx>; open list:SERIAL DRIVERS <linux- 
> serial@xxxxxxxxxxxxxxx>; Vadim Pasternak <vadimp@xxxxxxxxxxxx>; 
> system-sw-low-level <system-sw-low-level@xxxxxxxxxxxx>; Rob Herring 
> <robh+dt@xxxxxxxxxx>; openocd-devel-owner@xxxxxxxxxxxxxxxxxxxxx; 
> linux- api@xxxxxxxxxxxxxxx; David S. Miller <davem@xxxxxxxxxxxxx>; 
> Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>; Jiri Pirko 
> <jiri@xxxxxxxxxxxx>
> Subject: Re: [patch v21 1/4] drivers: jtag: Add JTAG core driver
> 
> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray 
> <oleksandrs@xxxxxxxxxxxx> wrote:
> > Initial patch for JTAG driver
> > JTAG class driver provide infrastructure to support 
> > hardware/software JTAG platform drivers. It provide user layer API 
> > interface for flashing and debugging external devices which equipped 
> > with JTAG interface using standard transactions.
> >
> > Driver exposes set of IOCTL to user space for:
> > - XFER:
> > - SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
> > - SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
> > - RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
> >   number of clocks).
> > - SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.
> >
> > Driver core provides set of internal APIs for allocation and
> > registration:
> > - jtag_register;
> > - jtag_unregister;
> > - jtag_alloc;
> > - jtag_free;
> >
> > Platform driver on registration with jtag-core creates the next 
> > entry in dev folder:
> > /dev/jtagX
> 
> >  0xB0   all     RATIO devices           in development:
> >                                         <mailto:vgo@xxxxxxxx>
> >  0xB1   00-1F   PPPoX                   <mailto:mostrows@xxxxxxxxxxxxxxxxx>
> > +0xB2   00-0f   linux/jtag.h            JTAG driver
> > +
> > +<mailto:oleksandrs@xxxxxxxxxxxx>
> 
> Consider to preserve style (upper vs. lower).

JTAG in code is lower (jtag) cane but in descriptions and notes it  is upper (JTAG).
In all places which do not correspond to this I will fix. 

> 
> > +         This provides basic core functionality support for JTAG class devices.
> > +         Hardware that is equipped with a JTAG microcontroller can be
> > +         supported by using this driver's interfaces.
> > +         This driver exposes a set of IOCTLs to the user space for
> > +         the following commands:
> > +         SDR: (Scan Data Register) Performs an IEEE 1149.1 Data Register scan
> > +         SIR: (Scan Instruction Register) Performs an IEEE 1149.1 Instruction
> > +         Register scan.
> > +         RUNTEST: Forces the IEEE 1149.1 bus to a run state for a specified
> > +         number of clocks or a specified time period.
> 
> Something feels wrong with formatting here.
> 

Will fix

> > +#define MAX_JTAG_NAME_LEN (sizeof("jtag") + 5)
> 
> Interesting definition. Why not to set to 10 explicitly? And why 10?
> (16 sounds better)
> 
5 - is a max len of JTAG device id in device name. I will add define to it.

In general I don't see the case for the system with hundreds JTAG interfaces.
I will limit maximum jtag master to 255 devices and change  id len to 3

#define MAX_JTAG_ID_STR_LEN 5
#define MAX_JTAG_NAME_LEN (sizeof("jtag") + MAX_JTAG_ID_STR_LEN)

> > +struct jtag {
> > +       struct miscdevice miscdev;
> 
> > +       struct device *dev;
> 
> Doesn't miscdev parent contain exactly this one?

Yes. 
Will fix.

> 
> > +       const struct jtag_ops *ops;
> > +       int id;
> > +       bool opened;
> > +       struct mutex open_lock;
> > +       unsigned long priv[0];
> > +};
> 
> > +               err = copy_to_user(u64_to_user_ptr(xfer.tdio),
> > +                                  (void *)(xfer_data), data_size);
> 
> Redundant parens in one case. Check the rest similar places.
> 

Yes.

> > +static int jtag_open(struct inode *inode, struct file *file) {
> 
> > +       struct jtag *jtag = container_of(file->private_data, struct jtag,
> > +                                        miscdev);
> 
> I would don't care about length and put it on one line.
> 

But following to LINUX kernel style, it should be no longer than 80 symbols. 
It will not pass by  ./scripts/checkpatch.pl

Will it be OK to send a patch which failed 80 symbols limitation check?

> > +       if (jtag->opened) {
> > +       jtag->opened = true;
> > +       jtag->opened = false;
> 
> Can it be opened non exclusively several times? If so, this needs to 
> be a ref counter instead.

It can be opened only once.

> 
> > +       if (!ops->idle || !ops->mode_set || !ops->status_get || !ops->xfer)
> > +               return NULL;
> 
> Are all of them mandatory?
> 

Yes, except "mode_set"
Will remove mode_set from check

> > +int jtag_register(struct jtag *jtag)
> 
> Perhaps devm_ variant.

Jtag driver uses miscdevice and related  misc_register and misc_deregister 
calls for creation and destruction. There is no device object prior to 
call to  misc_register, which could be used in devm_jtag_register.

> 
> > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
> 

Redundant. Removed.

> Where is this used or supposed to be used?
> 
> > +#define JTAG_MAX_XFER_DATA_LEN 65535
> 
> Is this limitation from some spec?
> Otherwise why not to allow 64K?
> 

It not limited by specification.  
But we enforce an upper bound for the length here, to prevent users from draining 
kernel memory with giant buffers.
So it was limited by  size of unsigned short int (65535)

> > +/**
> > + * struct jtag_ops - callbacks for jtag control functions:
> > + *
> > + * @freq_get: get frequency function. Filled by device driver
> > + * @freq_set: set frequency function. Filled by device driver
> > + * @status_get: set status function. Filled by device driver
> > + * @idle: set JTAG to idle state function. Filled by device driver
> > + * @xfer: send JTAG xfer function. Filled by device driver  */
> 
> Perhaps you need to describe which of them are _really_ mandatory and 
> which are optional.
> 

Ok

> --
> With Best Regards,
> Andy Shevchenko

Best Regards,
Oleksandr Shamray