Re: [PATCH v2 2/3] platform/chrome: Support reading/writing the vboot context

[Emilio LÃpez]

Hi Javier,

On 15/09/15 16:43, Javier Martinez Canillas wrote:
> Hello Emilio,
>
> On Tue, Sep 15, 2015 at 9:16 PM, Emilio LÃpez
> <emilio.lopez@xxxxxxxxxxxxxxx> wrote:
>
> [snip]
>
> > > >    obj-$(CONFIG_CHROMEOS_LAPTOP)  += chromeos_laptop.o
> > > >    obj-$(CONFIG_CHROMEOS_PSTORE)  += chromeos_pstore.o
> > > > -cros_ec_devs-objs               := cros_ec_dev.o cros_ec_sysfs.o
> > > > cros_ec_lightbar.o
> > > > +cros_ec_devs-objs               := cros_ec_dev.o
> > > > +cros_ec_devs-objs               += cros_ec_lightbar.o
> > > > +cros_ec_devs-objs               += cros_ec_sysfs.o
> > > > +cros_ec_devs-objs               += cros_ec_vbc.o
> > >
> > >
> > > Why are you changing the Makefile? AFAIK += is usually used when the
> > > compilation is conditional based on a Kconfig symbol but since these
> > > are build unconditionally, I'll just keep it as foo := bar baz
> >
> >
> > As far as I'm aware, += is append[0]. It's used for stuff like
> > obj-$(CONFIG_CHROMEOS_LAPTOP)  += chromeos_laptop.o
> > because the left part will resolve to "obj-y" or similar, and you want to
> > add to it, not replace it. I only changed the Makefile here because the line
> > was growing too long, and I thought it looked neater this way; it shouldn't
> > cause any functional change apart from the intended one.
> >
> > [0] https://www.gnu.org/software/make/manual/html_node/Appending.html
>
> Yes, I know how Kbuild works. What I tried to say is that you usually
> append based on a Kconfig symbol. In fact even you are mentioning such
> an example.
> So appending unconditionally like you are doing makes the Makefile
> harder to read IMHO. If the line grows to long you can use a backlash
> (\) char to split the line.

I guess it just boils down to personal preference; I don't feel that 
strongly about it, I'll change it in v3

(...)
> > > > +       struct device *dev = container_of(kobj, struct device, kobj);
> > > > +       struct cros_ec_dev *ec = container_of(dev, struct cros_ec_dev,
> > > > +                                             class_dev);
> > > > +       struct cros_ec_device *ecdev = ec->ec_dev;
> > > > +       struct ec_params_vbnvcontext *params;
> > > > +       struct cros_ec_command *msg;
> > > > +       int err;
> > > > +       const size_t para_sz = sizeof(struct ec_params_vbnvcontext);
> > > > +       const size_t resp_sz = sizeof(struct ec_response_vbnvcontext);
> > > > +       const size_t payload = max(para_sz, resp_sz);
> > > > +
> > > > +       msg = kmalloc(sizeof(*msg) + payload, GFP_KERNEL);
> > > > +       if (!msg)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       params = (struct ec_params_vbnvcontext *)msg->data;
> > > > +       params->op = EC_VBNV_CONTEXT_OP_READ;
> > > > +
> > > > +       msg->version = EC_VER_VBNV_CONTEXT;
> > > > +       msg->command = EC_CMD_VBNV_CONTEXT;
> > > > +       msg->outsize = sizeof(params->op);
> > >
> > >
> > > Shouldn't this be para_sz ? Since you are sending to the EC the whole
> > > struct ec_params_vbnvcontext and not only the op field.
> > >
> > > Or if the EC only expects to get the u32 op field, then I think your
> > > max payload calculation is not correct.
> >
> >
> > The params struct is the same for both read and write ops, so it has the op
>
> That's not true, struct ec_response_vbnvcontext has only the block
> field while struct ec_param_vbnvcontext has both the op and block
> fields.

The former is a response struct, not a params struct.

> > flag and a buffer for the write op. During the read op I believe there's no
> > need to send this potentially-garbage-filled buffer to the EC, so outsize is
> > set accordingly here.
>
> Yes, I agree with you but then as I mentioned I think your payload
> calculation is wrong since you want instead max(sizeof(struct
> ec_response_vbnvcontext), sizeof(param->op)). Otherwise you are
> allocating 4 bytes more than needed.

Yeah, I can see that. If I do that though, max(...) would be less than 
the size of the full params struct, and casting data to it could lead to 
subtle bugs in the future. I can change it and add a comment mentioning 
this, deal?

(...)

> with the needed changes, feel free to add my:
>
> Reviewed-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>

Ok, thanks!

Emilio
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/