linux-kernel - RE: [PATCH v3 3/9] bus: fsl-mc: dpio: add APIs for DPIO objects

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <VI1PR0401MB26389555343B2A2AD09541698D9D0@VI1PR0401MB2638.eurprd04.prod.outlook.com>
Date:   Thu, 15 Dec 2016 16:36:43 +0000
From:   Stuart Yoder <stuart.yoder@....com>
To:     Laurentiu Tudor <laurentiu.tudor@....com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "agraf@...e.de" <agraf@...e.de>, "arnd@...db.de" <arnd@...db.de>,
        Leo Li <leoyang.li@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        "Catalin Horghidan" <catalin.horghidan@....com>,
        Ruxandra Ioana Radulescu <ruxandra.radulescu@....com>,
        Roy Pledge <roy.pledge@....com>
Subject: RE: [PATCH v3 3/9] bus: fsl-mc: dpio: add APIs for DPIO objects

> > +#define DPIO_CMD(id)	((id << DPIO_CMD_ID_OFFSET) | DPIO_CMD_BASE_VERSION)
> 
> Paranthesis around 'id'?

In all cases these are opcode values and will never be an expression.  If
we really need to future proof these definitions, we should do it for all
objects not just DPIO.  I'd like to see consistency across objects and don't
want to see DPIO gratuitously diverge.  So, my suggestion is to have an
offline discussion and if we think the change is needed, submit a patch for
all objects currently supported.

> > +	/* prepare command */
> > +	cmd.header = mc_encode_cmd_header(DPIO_CMDID_OPEN,
> > +					  cmd_flags,
> > +					  0);
> > +	dpio_cmd = (struct dpio_cmd_open *)cmd.params;
> > +	dpio_cmd->dpio_id = cpu_to_le32(dpio_id);
> > +
> > +	/* send command to mc*/
> > +	err = mc_send_command(mc_io, &cmd);
> > +	if (err)
> > +		return err;
> > +
> > +	/* retrieve response parameters */
> > +	*token = mc_cmd_hdr_read_token(&cmd);
> 
> Nit: maybe we should drop these repetitive "prepare / send / retrieve" comments
> as the code is pretty self explanatory.

The 'send' comment certainly isn't needed given that the function
is 'mc_send_command()'.  For the others, I think having some comment
is helpful, even though a bit repetitive.

Stuart