Re: [PATCH v5 07/22] xen/arm: ITS: Add virtual ITS driver

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-07-31 at 11:14 +0100, Julien Grall wrote:
> Hi Vijay,
> 
> On 31/07/15 07:49, Vijay Kilari wrote:
> > > > +static int vits_vitt_entry(struct domain *d, uint32_t devid,
> > > > +                           uint32_t event, struct vitt *entry, 
> > > > bool_t set)
> > > > +{
> > > > +    struct vdevice_table dt_entry;
> > > > +    paddr_t vitt_entry;
> > > > +    uint64_t offset;
> > > > +
> > > > +    BUILD_BUG_ON(sizeof(struct vitt) != 8);
> > > > +
> > > > +    if ( vits_get_vdevice_entry(d, devid, &dt_entry) )
> > > > +    {
> > > > +        printk(XENLOG_G_ERR
> > > > +                "d%"PRId32": vITS: Fail to get vdevice for vdev 
> > > > 0x%"PRIx32"\n",
> > > 
> > > s/vdev/vdevid/
> > 
> > I think, to manage within 80 char, I used just "vdev"
> 
> 80 character in the source file I guess? If so, you should avoid this
> kind of cutting just for coding style benefits. We are not so strict on 
> it.

It's also acceptable to indent string constants by less than what would be
expected to avoid this, e.g.
        printk(XENLOG_G_ERR
         d%"PRId32": vITS: Fail to get vdevice for vdevid 0x%"PRIx32"\n",

(also, s/Fail/Failed/ or perhaps "Unable").

Also, I don't think PRId32 is what we should use for d->domain_id. The
actual type is uint16_t, which would suggest PRIu16 (or maybe PRId16) is
formally correct but in actuality we normally use %u or %d in this case (%u
is probably more correct, although it appears to be in the minority).

> > > > +                d->domain_id, devid);
> > > > +        return -EINVAL;
> > > > +    }
> > > > +
> > > > +    /* dt_entry is validated in vits_get_vdevice_entry */
> > > 
> > > s/is validated/has been validated/
> > > 
> > > [..]
> > > 
> > > > +int vits_set_vitt_entry(struct domain *d, uint32_t devid,
> > > > +                        uint32_t event, struct vitt *entry)
> > > 
> > > Same remark as vits_set_vdevice_entry.
> > 
> > I have made non-static for compilation purpose. I will try to introduce
> > this in the patch where it is used. But it is more logical to have this
> > in this patch. Anyway I forget to make it static in later patches
> 
> Having introduce static here would have avoid forgetting the static
> later... It's just a matter of how you split your series.
> 
> For instance, if you would have merge this patch with #8, making this
> function non-static wouldn't have been necessary.

I've also suggest elsewhere not adding these new files to the build until
later in the series, such that the user of this function is already added
by the time the file starts to get compiled.

That approach is usually acceptable when introducing a large new file which
isn't used until towards the end of the series.

Of course you need to avoid calling any functions in that file until after
you add it to the build, which introduces a different constraint on the
series ordering, so it's not always possible to make use of this approach.

Ian.