RE: [PATCH 03/10] soc: fujitsu: hwb: Add IOC_BB_ALLOC ioctl

["misono.tomohiro@fujitsu.com" <misono.tomohiro@fujitsu.com>]

> On Fri, Jan 8, 2021 at 11:52 AM Misono Tomohiro
> <misono.tomohiro@jp.fujitsu.com> wrote:
> 
> > +static void write_init_sync_reg(void *args)
> > +{
> > +       struct init_sync_args *sync_args = (struct init_sync_args *)args;
> > +
> > +       switch (sync_args->bb) {
> > +       case 0:
> > +               write_sysreg_s(sync_args->val, FHWB_INIT_SYNC_BB0_EL1);
> > +               break;
> > +       case 1:
> > +               write_sysreg_s(sync_args->val, FHWB_INIT_SYNC_BB1_EL1);
> > +               break;
> > +       case 2:
> > +               write_sysreg_s(sync_args->val, FHWB_INIT_SYNC_BB2_EL1);
> > +               break;
> > +       case 3:
> > +               write_sysreg_s(sync_args->val, FHWB_INIT_SYNC_BB3_EL1);
> > +               break;
> > +       case 4:
> > +               write_sysreg_s(sync_args->val, FHWB_INIT_SYNC_BB4_EL1);
> > +               break;
> > +       case 5:
> > +               write_sysreg_s(sync_args->val, FHWB_INIT_SYNC_BB5_EL1);
> > +               break;
> > +       }
> > +}
> 
> (minor style comment
> 
> I think this could be simplified into a single write_sysreg_s() with the
> register number calculated based on sync_args->bb, rather than duplicating
> the same three lines six times.

I think msr/mrs instructions needs register names at compile time so
it cannot be calculate dynamically. Or am I misunderstood?

> > +static int ioc_bb_alloc(struct file *filp, void __user *argp)
> > +{
> > +       struct hwb_private_data *pdata = (struct hwb_private_data *)filp->private_data;
> 
> A slightly better way to write the ioctl command specific functions
> is to just give the argument the correct type (struct
> fujitsu_hwb_ioc_bb_ctl __user*)
> instead of 'void __user *', to avoid the cast.
> 
> Similarly, as you don't use 'filp' itself, just pass the struct hwb_private_data
> pointer as the first argument.

thanks, I will follow this advise.
 
> > @@ -164,6 +386,7 @@ static int fujitsu_hwb_dev_open(struct inode *inode, struct file *filp)
> >  static const struct file_operations fujitsu_hwb_dev_fops = {
> >         .owner          = THIS_MODULE,
> >         .open           = fujitsu_hwb_dev_open,
> > +       .unlocked_ioctl = fujitsu_hwb_dev_ioctl,
> >  };
> 
> All drivers with an ioctl interface should work in compat mode as well,
> so please add a
> 
>        .compat_ioctl = compat_ptr_ioctl;


A64FX does not support 32-bit mode (aarch32 state).
So I think unlockd_ioctl is enough or is it better to use compat_ioctl anyway?

> 
> > +#define __FUJITSU_IOCTL_MAGIC 'F'
> 
> It's hard to find a non-conflicting range of ioctl numbers, but
> it would be good to note the conflict in
> 
> Documentation/userspace-api/ioctl/ioctl-number.rst
> 
> The 'F' range is also used in framebuffer drivers.

I didn't notice this, thanks for pointing out.

Again, thanks for all the reviews/comments.
Tomohiro

> > +/* ioctl definitions for hardware barrier driver */
> > +struct fujitsu_hwb_ioc_bb_ctl {
> > +       __u8 cmg;
> > +       __u8 bb;
> > +       __u8 unused[2];
> > +       __u32 size;
> > +       unsigned long __user *pemask;
> > +};
> 
> However, this structure layout is incompatible with 32-bit user
> space because of the indirect pointer. See
> Documentation/driver-api/ioctl.rst for how to encode a
> pointer in a __u64 member.