| Message ID | 544859b5-108a-1909-d612-64f67a02aeec@free.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] media: si2168: Refactor command setup code | expand |
Hello, On Thu, Jul 04, 2019 at 12:33:22PM +0200, Marc Gonzalez wrote: > Refactor the command setup code, and let the compiler determine > the size of each command. > > Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > --- > Changes from v1: > - Use a real function to populate struct si2168_cmd *cmd, and a trivial > macro wrapping it (macro because sizeof). > Changes from v2: > - Fix header mess > - Add Jonathan's tag > --- > drivers/media/dvb-frontends/si2168.c | 146 +++++++++------------------ > 1 file changed, 45 insertions(+), 101 deletions(-) > > diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c > index c64b360ce6b5..5e81e076369c 100644 > --- a/drivers/media/dvb-frontends/si2168.c > +++ b/drivers/media/dvb-frontends/si2168.c > @@ -12,6 +12,16 @@ > > static const struct dvb_frontend_ops si2168_ops; > > +static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen) I'd add an "inline" here. And you could add a const for *args. > +{ > + memcpy(cmd->args, args, wlen); > + cmd->wlen = wlen; > + cmd->rlen = rlen; > +} > + > +#define CMD_SETUP(cmd, args, rlen) \ > + cmd_setup(cmd, args, sizeof(args) - 1, rlen) Here is the chance to add some static checking. Also it is a good habit to put parens around macro arguments. Something like: #define CMD_SETUP(cmd, args, rlen) ({ \ BUILD_BUG_ON(sizeof((args)) - 1 > SI2168_ARGLEN); cmd_setup((cmd), (args), __must_be_array((args)) + sizeof((args)) - 1, (rlen)); Maybe let this macro live in drivers/media/dvb-frontends/si2168_priv.h where struct si2168_cmd is defined? I looked over the transformations in the rest of the patch and this looks good. Best regards Uwe
+ Sean On 12/07/2019 10:43, Uwe Kleine-König wrote: > On Thu, Jul 04, 2019 at 12:33:22PM +0200, Marc Gonzalez wrote: > >> Refactor the command setup code, and let the compiler determine >> the size of each command. >> >> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> >> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> >> --- >> Changes from v1: >> - Use a real function to populate struct si2168_cmd *cmd, and a trivial >> macro wrapping it (macro because sizeof). >> Changes from v2: >> - Fix header mess >> - Add Jonathan's tag >> --- >> drivers/media/dvb-frontends/si2168.c | 146 +++++++++------------------ >> 1 file changed, 45 insertions(+), 101 deletions(-) >> >> diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c >> index c64b360ce6b5..5e81e076369c 100644 >> --- a/drivers/media/dvb-frontends/si2168.c >> +++ b/drivers/media/dvb-frontends/si2168.c >> @@ -12,6 +12,16 @@ >> >> static const struct dvb_frontend_ops si2168_ops; >> >> +static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen) > > I'd add an "inline" here. And you could add a const for *args. I was under the (vague) impression that it's better to let the compiler decide when to inline, except for trivial alternatives in headers. David Miller wrote: "Please do not use the inline directive in foo.c files, let the compiler decide." Antti, Sean, what do you think? For my notes: https://gcc.gnu.org/onlinedocs/gcc/Inline.html >> +{ >> + memcpy(cmd->args, args, wlen); >> + cmd->wlen = wlen; >> + cmd->rlen = rlen; >> +} >> + >> +#define CMD_SETUP(cmd, args, rlen) \ >> + cmd_setup(cmd, args, sizeof(args) - 1, rlen) > > Here is the chance to add some static checking. Also it is a good habit > to put parens around macro arguments. Wrt parens around arguments, I figured they are not required here, since they are used as function arguments. Though you may have a valid point. Antti, Sean? > Something like: > > #define CMD_SETUP(cmd, args, rlen) ({ \ > BUILD_BUG_ON(sizeof((args)) - 1 > SI2168_ARGLEN); > cmd_setup((cmd), (args), __must_be_array((args)) + sizeof((args)) - 1, (rlen)); > > Maybe let this macro live in drivers/media/dvb-frontends/si2168_priv.h > where struct si2168_cmd is defined? Antti, Sean? > I looked over the transformations in the rest of the patch and this > looks good. Thanks for taking a look! Regards.
Hi Marc, Replying inline. On 04/07/2019 05.33, Marc Gonzalez wrote: > Refactor the command setup code, and let the compiler determine > the size of each command. > > Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > --- > Changes from v1: > - Use a real function to populate struct si2168_cmd *cmd, and a trivial > macro wrapping it (macro because sizeof). > Changes from v2: > - Fix header mess > - Add Jonathan's tag > --- > drivers/media/dvb-frontends/si2168.c | 146 +++++++++------------------ > 1 file changed, 45 insertions(+), 101 deletions(-) > > diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c > index c64b360ce6b5..5e81e076369c 100644 > --- a/drivers/media/dvb-frontends/si2168.c > +++ b/drivers/media/dvb-frontends/si2168.c > @@ -12,6 +12,16 @@ > > static const struct dvb_frontend_ops si2168_ops; > > +static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen) > +{ > + memcpy(cmd->args, args, wlen); > + cmd->wlen = wlen; > + cmd->rlen = rlen; > +} > + struct si2168_cmd.args is u8, not char. I also think const should apply to the pointer. > +#define CMD_SETUP(cmd, args, rlen) \ > + cmd_setup(cmd, args, sizeof(args) - 1, rlen) > + This is only a valid helper if args is a null terminated string. It just so happens that every instance in this driver is, but that could be a silent pitfall if someone used a u8 array with this macro. Otherwise I'm ok with the refactoring. Regards, Brad > /* execute firmware command */ > static int si2168_cmd_execute(struct i2c_client *client, struct si2168_cmd *cmd) > { > @@ -84,15 +94,13 @@ static int si2168_ts_bus_ctrl(struct dvb_frontend *fe, int acquire) > dev_dbg(&client->dev, "%s acquire: %d\n", __func__, acquire); > > /* set TS_MODE property */ > - memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6); > + CMD_SETUP(&cmd, "\x14\x00\x01\x10\x10\x00", 4); > if (acquire) > cmd.args[4] |= dev->ts_mode; > else > cmd.args[4] |= SI2168_TS_TRISTATE; > if (dev->ts_clock_gapped) > cmd.args[4] |= 0x40; > - cmd.wlen = 6; > - cmd.rlen = 4; > ret = si2168_cmd_execute(client, &cmd); > > return ret; > @@ -116,19 +124,13 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status) > > switch (c->delivery_system) { > case SYS_DVBT: > - memcpy(cmd.args, "\xa0\x01", 2); > - cmd.wlen = 2; > - cmd.rlen = 13; > + CMD_SETUP(&cmd, "\xa0\x01", 13); > break; > case SYS_DVBC_ANNEX_A: > - memcpy(cmd.args, "\x90\x01", 2); > - cmd.wlen = 2; > - cmd.rlen = 9; > + CMD_SETUP(&cmd, "\x90\x01", 9); > break; > case SYS_DVBT2: > - memcpy(cmd.args, "\x50\x01", 2); > - cmd.wlen = 2; > - cmd.rlen = 14; > + CMD_SETUP(&cmd, "\x50\x01", 14); > break; > default: > ret = -EINVAL; > @@ -165,9 +167,7 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status) > > /* BER */ > if (*status & FE_HAS_VITERBI) { > - memcpy(cmd.args, "\x82\x00", 2); > - cmd.wlen = 2; > - cmd.rlen = 3; > + CMD_SETUP(&cmd, "\x82\x00", 3); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > @@ -198,9 +198,7 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status) > > /* UCB */ > if (*status & FE_HAS_SYNC) { > - memcpy(cmd.args, "\x84\x01", 2); > - cmd.wlen = 2; > - cmd.rlen = 3; > + CMD_SETUP(&cmd, "\x84\x01", 3); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > @@ -286,22 +284,18 @@ static int si2168_set_frontend(struct dvb_frontend *fe) > goto err; > } > > - memcpy(cmd.args, "\x88\x02\x02\x02\x02", 5); > - cmd.wlen = 5; > - cmd.rlen = 5; > + CMD_SETUP(&cmd, "\x88\x02\x02\x02\x02", 5); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > /* that has no big effect */ > if (c->delivery_system == SYS_DVBT) > - memcpy(cmd.args, "\x89\x21\x06\x11\xff\x98", 6); > + CMD_SETUP(&cmd, "\x89\x21\x06\x11\xff\x98", 3); > else if (c->delivery_system == SYS_DVBC_ANNEX_A) > - memcpy(cmd.args, "\x89\x21\x06\x11\x89\xf0", 6); > + CMD_SETUP(&cmd, "\x89\x21\x06\x11\x89\xf0", 3); > else if (c->delivery_system == SYS_DVBT2) > - memcpy(cmd.args, "\x89\x21\x06\x11\x89\x20", 6); > - cmd.wlen = 6; > - cmd.rlen = 3; > + CMD_SETUP(&cmd, "\x89\x21\x06\x11\x89\x20", 3); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > @@ -318,103 +312,77 @@ static int si2168_set_frontend(struct dvb_frontend *fe) > goto err; > } > > - memcpy(cmd.args, "\x51\x03", 2); > - cmd.wlen = 2; > - cmd.rlen = 12; > + CMD_SETUP(&cmd, "\x51\x03", 12); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > - memcpy(cmd.args, "\x12\x08\x04", 3); > - cmd.wlen = 3; > - cmd.rlen = 3; > + CMD_SETUP(&cmd, "\x12\x08\x04", 3); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > - memcpy(cmd.args, "\x14\x00\x0c\x10\x12\x00", 6); > - cmd.wlen = 6; > - cmd.rlen = 4; > + CMD_SETUP(&cmd, "\x14\x00\x0c\x10\x12\x00", 4); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > - memcpy(cmd.args, "\x14\x00\x06\x10\x24\x00", 6); > - cmd.wlen = 6; > - cmd.rlen = 4; > + CMD_SETUP(&cmd, "\x14\x00\x06\x10\x24\x00", 4); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > - memcpy(cmd.args, "\x14\x00\x07\x10\x00\x24", 6); > - cmd.wlen = 6; > - cmd.rlen = 4; > + CMD_SETUP(&cmd, "\x14\x00\x07\x10\x00\x24", 4); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > - memcpy(cmd.args, "\x14\x00\x0a\x10\x00\x00", 6); > + CMD_SETUP(&cmd, "\x14\x00\x0a\x10\x00\x00", 4); > cmd.args[4] = delivery_system | bandwidth; > if (dev->spectral_inversion) > cmd.args[5] |= 1; > - cmd.wlen = 6; > - cmd.rlen = 4; > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > /* set DVB-C symbol rate */ > if (c->delivery_system == SYS_DVBC_ANNEX_A) { > - memcpy(cmd.args, "\x14\x00\x02\x11", 4); > + CMD_SETUP(&cmd, "\x14\x00\x02\x11\x00\x00", 4); > cmd.args[4] = ((c->symbol_rate / 1000) >> 0) & 0xff; > cmd.args[5] = ((c->symbol_rate / 1000) >> 8) & 0xff; > - cmd.wlen = 6; > - cmd.rlen = 4; > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > } > > - memcpy(cmd.args, "\x14\x00\x0f\x10\x10\x00", 6); > - cmd.wlen = 6; > - cmd.rlen = 4; > + CMD_SETUP(&cmd, "\x14\x00\x0f\x10\x10\x00", 4); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > - memcpy(cmd.args, "\x14\x00\x09\x10\xe3\x08", 6); > + CMD_SETUP(&cmd, "\x14\x00\x09\x10\xe3\x08", 4); > cmd.args[5] |= dev->ts_clock_inv ? 0x00 : 0x10; > - cmd.wlen = 6; > - cmd.rlen = 4; > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > - memcpy(cmd.args, "\x14\x00\x08\x10\xd7\x05", 6); > + CMD_SETUP(&cmd, "\x14\x00\x08\x10\xd7\x05", 4); > cmd.args[5] |= dev->ts_clock_inv ? 0x00 : 0x10; > - cmd.wlen = 6; > - cmd.rlen = 4; > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > - memcpy(cmd.args, "\x14\x00\x01\x12\x00\x00", 6); > - cmd.wlen = 6; > - cmd.rlen = 4; > + CMD_SETUP(&cmd, "\x14\x00\x01\x12\x00\x00", 4); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > - memcpy(cmd.args, "\x14\x00\x01\x03\x0c\x00", 6); > - cmd.wlen = 6; > - cmd.rlen = 4; > + CMD_SETUP(&cmd, "\x14\x00\x01\x03\x0c\x00", 4); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > - memcpy(cmd.args, "\x85", 1); > - cmd.wlen = 1; > - cmd.rlen = 1; > + CMD_SETUP(&cmd, "\x85", 1); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > @@ -444,26 +412,20 @@ static int si2168_init(struct dvb_frontend *fe) > dev_dbg(&client->dev, "\n"); > > /* initialize */ > - memcpy(cmd.args, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13); > - cmd.wlen = 13; > - cmd.rlen = 0; > + CMD_SETUP(&cmd, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 0); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > if (dev->warm) { > /* resume */ > - memcpy(cmd.args, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 8); > - cmd.wlen = 8; > - cmd.rlen = 1; > + CMD_SETUP(&cmd, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 1); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > udelay(100); > - memcpy(cmd.args, "\x85", 1); > - cmd.wlen = 1; > - cmd.rlen = 1; > + CMD_SETUP(&cmd, "\x85", 1); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > @@ -472,9 +434,7 @@ static int si2168_init(struct dvb_frontend *fe) > } > > /* power up */ > - memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8); > - cmd.wlen = 8; > - cmd.rlen = 1; > + CMD_SETUP(&cmd, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 1); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > @@ -542,17 +502,13 @@ static int si2168_init(struct dvb_frontend *fe) > > release_firmware(fw); > > - memcpy(cmd.args, "\x01\x01", 2); > - cmd.wlen = 2; > - cmd.rlen = 1; > + CMD_SETUP(&cmd, "\x01\x01", 1); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > /* query firmware version */ > - memcpy(cmd.args, "\x11", 1); > - cmd.wlen = 1; > - cmd.rlen = 10; > + CMD_SETUP(&cmd, "\x11", 10); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > @@ -610,9 +566,7 @@ static int si2168_sleep(struct dvb_frontend *fe) > if (dev->version > ('B' << 24 | 4 << 16 | 0 << 8 | 11 << 0)) > dev->warm = false; > > - memcpy(cmd.args, "\x13", 1); > - cmd.wlen = 1; > - cmd.rlen = 0; > + CMD_SETUP(&cmd, "\x13", 0); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > @@ -638,9 +592,7 @@ static int si2168_select(struct i2c_mux_core *muxc, u32 chan) > struct si2168_cmd cmd; > > /* open I2C gate */ > - memcpy(cmd.args, "\xc0\x0d\x01", 3); > - cmd.wlen = 3; > - cmd.rlen = 0; > + CMD_SETUP(&cmd, "\xc0\x0d\x01", 0); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > @@ -658,9 +610,7 @@ static int si2168_deselect(struct i2c_mux_core *muxc, u32 chan) > struct si2168_cmd cmd; > > /* close I2C gate */ > - memcpy(cmd.args, "\xc0\x0d\x00", 3); > - cmd.wlen = 3; > - cmd.rlen = 0; > + CMD_SETUP(&cmd, "\xc0\x0d\x00", 0); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > @@ -753,25 +703,19 @@ static int si2168_probe(struct i2c_client *client, > mutex_init(&dev->i2c_mutex); > > /* Initialize */ > - memcpy(cmd.args, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13); > - cmd.wlen = 13; > - cmd.rlen = 0; > + CMD_SETUP(&cmd, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 0); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err_kfree; > > /* Power up */ > - memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8); > - cmd.wlen = 8; > - cmd.rlen = 1; > + CMD_SETUP(&cmd, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 1); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err_kfree; > > /* Query chip revision */ > - memcpy(cmd.args, "\x02", 1); > - cmd.wlen = 1; > - cmd.rlen = 13; > + CMD_SETUP(&cmd, "\x02", 13); > ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err_kfree;
Em Fri, 12 Jul 2019 10:47:17 -0500 Brad Love <brad@nextdimension.cc> escreveu: > Hi Marc, > > Replying inline. > > > On 04/07/2019 05.33, Marc Gonzalez wrote: > > Refactor the command setup code, and let the compiler determine > > the size of each command. > > > > Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > > --- > > Changes from v1: > > - Use a real function to populate struct si2168_cmd *cmd, and a trivial > > macro wrapping it (macro because sizeof). > > Changes from v2: > > - Fix header mess > > - Add Jonathan's tag > > --- > > drivers/media/dvb-frontends/si2168.c | 146 +++++++++------------------ > > 1 file changed, 45 insertions(+), 101 deletions(-) > > > > diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c > > index c64b360ce6b5..5e81e076369c 100644 > > --- a/drivers/media/dvb-frontends/si2168.c > > +++ b/drivers/media/dvb-frontends/si2168.c > > @@ -12,6 +12,16 @@ > > > > static const struct dvb_frontend_ops si2168_ops; > > > > +static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen) > > +{ > > + memcpy(cmd->args, args, wlen); > > + cmd->wlen = wlen; > > + cmd->rlen = rlen; > > +} > > + > > > struct si2168_cmd.args is u8, not char. I also think const should apply > to the pointer. > > > > +#define CMD_SETUP(cmd, args, rlen) \ > > + cmd_setup(cmd, args, sizeof(args) - 1, rlen) > > + > > > This is only a valid helper if args is a null terminated string. It just > so happens that every instance in this driver is, but that could be a > silent pitfall if someone used a u8 array with this macro. Actually, it is uglier than that. Of one writes something like: char buf[20]; buf[0] = 0x20; buf[1] = 0x03; CMD_SETUP(cmd, buf, 0); // some other init, up to 5 values, then another CMD_SETUP() sizeof() will evaluate to 20, and not to 2, with would be the expected buffer size, and it will pass 18 random values. IMHO, using sizeof() here is a very bad idea. Regards, Mauro
On 12/07/2019 17:47, Brad Love wrote: > On 04/07/2019 05.33, Marc Gonzalez wrote: > >> Refactor the command setup code, and let the compiler determine >> the size of each command. >> >> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> >> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> >> --- >> Changes from v1: >> - Use a real function to populate struct si2168_cmd *cmd, and a trivial >> macro wrapping it (macro because sizeof). >> Changes from v2: >> - Fix header mess >> - Add Jonathan's tag >> --- >> drivers/media/dvb-frontends/si2168.c | 146 +++++++++------------------ >> 1 file changed, 45 insertions(+), 101 deletions(-) >> >> diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c >> index c64b360ce6b5..5e81e076369c 100644 >> --- a/drivers/media/dvb-frontends/si2168.c >> +++ b/drivers/media/dvb-frontends/si2168.c >> @@ -12,6 +12,16 @@ >> >> static const struct dvb_frontend_ops si2168_ops; >> >> +static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen) >> +{ >> + memcpy(cmd->args, args, wlen); >> + cmd->wlen = wlen; >> + cmd->rlen = rlen; >> +} > > struct si2168_cmd.args is u8, not char. Yes, struct si2168_cmd.args is an u8 array. However, string literals such as "\xa0\x01" are char arrays. memcpy() ignores the types altogether. > I also think const should apply to the pointer. I can do that, even though it's obvious we're not writing to args in the trivial cmd_setup() body. >> +#define CMD_SETUP(cmd, args, rlen) \ >> + cmd_setup(cmd, args, sizeof(args) - 1, rlen) >> + > > This is only a valid helper if args is a null terminated string. You say that because of the "-1" arithmetic, I assume? > It just so happens that every instance in this driver is, FWIW, there are 2 calls where it is not. memcpy(cmd.args, &fw->data[(fw->size - remaining) + 1], len); memcpy(cmd.args, &fw->data[fw->size - remaining], len); > but that could be a silent pitfall if someone used a u8 array > with this macro. Actually, the compiler warns if we pass an u8 array instead of a char array. IMO, the type is actually a good thing, since it warns for cases where we don't use a string literal. > Otherwise I'm ok with the refactoring. I'll see what I can do... Regards.
On 12/07/2019 19:45, Mauro Carvalho Chehab wrote: > Brad Love <brad@nextdimension.cc> escreveu: > >> On 04/07/2019 05.33, Marc Gonzalez wrote: >> >>> +#define CMD_SETUP(cmd, args, rlen) \ >>> + cmd_setup(cmd, args, sizeof(args) - 1, rlen) >>> + >> >> This is only a valid helper if args is a null terminated string. It just >> so happens that every instance in this driver is, but that could be a >> silent pitfall if someone used a u8 array with this macro. > > Actually, it is uglier than that. If one writes something like: > > char buf[20]; > > buf[0] = 0x20; > buf[1] = 0x03; > > CMD_SETUP(cmd, buf, 0); > > // some other init, up to 5 values, then another CMD_SETUP() I'm not sure what you mean in the // comment. What kind of init? Why up to 5 values? Why another CMD_SETUP? > sizeof() will evaluate to 20, and not to 2, with would be the > expected buffer size, and it will pass 18 random values. > > IMHO, using sizeof() here is a very bad idea. You may have a point... (Though I'm not proposing a kernel API function, merely code refactoring for a single file that's unlikely to change going forward.) It's also bad form to repeat the cmd size (twice) when the compiler can figure it out automatically for string literals (which is 95% of the use-cases). I can drop the macro, and just use the helper... Or maybe there's a GCC extension to test that an argument is a string literal... Regards.
Em Sat, 13 Jul 2019 00:11:12 +0200 Marc Gonzalez <marc.w.gonzalez@free.fr> escreveu: > On 12/07/2019 19:45, Mauro Carvalho Chehab wrote: > > > Brad Love <brad@nextdimension.cc> escreveu: > > > >> On 04/07/2019 05.33, Marc Gonzalez wrote: > >> > >>> +#define CMD_SETUP(cmd, args, rlen) \ > >>> + cmd_setup(cmd, args, sizeof(args) - 1, rlen) > >>> + > >> > >> This is only a valid helper if args is a null terminated string. It just > >> so happens that every instance in this driver is, but that could be a > >> silent pitfall if someone used a u8 array with this macro. > > > > Actually, it is uglier than that. If one writes something like: > > > > char buf[20]; > > > > buf[0] = 0x20; > > buf[1] = 0x03; > > > > CMD_SETUP(cmd, buf, 0); > > > > // some other init, up to 5 values, then another CMD_SETUP() > > I'm not sure what you mean in the // comment. > What kind of init? Why up to 5 values? Why another CMD_SETUP? I mean that the same buffer could be re-used to do something like: char buf[20]; buf[0] = 0x20; buf[1] = 0x03; CMD_SETUP(cmd, buf, 0); // write size here should be 2 buf[2] = 0x04 buf[3] = 0x00 buf[4] = 0x05 CMD_SETUP(cmd, buf, 0); // write size here should be 5 This kind of pattern happens on other drivers and someone may end needing something like that at this driver on some future. > > sizeof() will evaluate to 20, and not to 2, with would be the > > expected buffer size, and it will pass 18 random values. > > > > IMHO, using sizeof() here is a very bad idea. > > You may have a point... > (Though I'm not proposing a kernel API function, merely code > refactoring for a single file that's unlikely to change going > forward.) Yes, I know, but we had already some bugs due to the usage of sizeof() on similar macros at drivers in the past. > It's also bad form to repeat the cmd size (twice) when the compiler > can figure it out automatically for string literals (which is 95% > of the use-cases). > > I can drop the macro, and just use the helper... The helper function sounds fine. > > Or maybe there's a GCC extension to test that an argument is a > string literal... If this could be evaluated by some advanced macro logic that would work not only with gcc but also with clang, then a macro that does what you proposed could be useful. There are some ways to check the type of a macro argument, but I'm not sure if are there any way for it to distinguish between a string constant from a char * array. Thanks, Mauro
Am 13.07.19 um 12:02 schrieb Mauro Carvalho Chehab: > Em Sat, 13 Jul 2019 00:11:12 +0200 > Marc Gonzalez <marc.w.gonzalez@free.fr> escreveu: > >> On 12/07/2019 19:45, Mauro Carvalho Chehab wrote: >> >>> Brad Love <brad@nextdimension.cc> escreveu: >>> >>> IMHO, using sizeof() here is a very bad idea. >> >> You may have a point... >> (Though I'm not proposing a kernel API function, merely code >> refactoring for a single file that's unlikely to change going >> forward.) > > Yes, I know, but we had already some bugs due to the usage of > sizeof() on similar macros at drivers in the past. > >> It's also bad form to repeat the cmd size (twice) when the compiler >> can figure it out automatically for string literals (which is 95% >> of the use-cases). >> >> I can drop the macro, and just use the helper... > > The helper function sounds fine. > >> >> Or maybe there's a GCC extension to test that an argument is a >> string literal... > > If this could be evaluated by some advanced macro logic that > would work not only with gcc but also with clang, then a > macro that does what you proposed could be useful. > > There are some ways to check the type of a macro argument, but I'm > not sure if are there any way for it to distinguish between a > string constant from a char * array. > Maybe something like this will prevent compilation if the argument is no string literal: #define CMD_SETUP(cmd, args, rlen) \ cmd_setup(cmd, args "", sizeof(args) - 1, rlen) Another idea is a check like: #define CMD_SETUP(cmd, args, rlen) \ do { \ BUILD_BUG_ON(#args[0] != "\""); \ cmd_setup(cmd, args "", sizeof(args) - 1, rlen) \ } while(0) Regards Matthias
diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c index c64b360ce6b5..5e81e076369c 100644 --- a/drivers/media/dvb-frontends/si2168.c +++ b/drivers/media/dvb-frontends/si2168.c @@ -12,6 +12,16 @@ static const struct dvb_frontend_ops si2168_ops; +static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen) +{ + memcpy(cmd->args, args, wlen); + cmd->wlen = wlen; + cmd->rlen = rlen; +} + +#define CMD_SETUP(cmd, args, rlen) \ + cmd_setup(cmd, args, sizeof(args) - 1, rlen) + /* execute firmware command */ static int si2168_cmd_execute(struct i2c_client *client, struct si2168_cmd *cmd) { @@ -84,15 +94,13 @@ static int si2168_ts_bus_ctrl(struct dvb_frontend *fe, int acquire) dev_dbg(&client->dev, "%s acquire: %d\n", __func__, acquire); /* set TS_MODE property */ - memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6); + CMD_SETUP(&cmd, "\x14\x00\x01\x10\x10\x00", 4); if (acquire) cmd.args[4] |= dev->ts_mode; else cmd.args[4] |= SI2168_TS_TRISTATE; if (dev->ts_clock_gapped) cmd.args[4] |= 0x40; - cmd.wlen = 6; - cmd.rlen = 4; ret = si2168_cmd_execute(client, &cmd); return ret; @@ -116,19 +124,13 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status) switch (c->delivery_system) { case SYS_DVBT: - memcpy(cmd.args, "\xa0\x01", 2); - cmd.wlen = 2; - cmd.rlen = 13; + CMD_SETUP(&cmd, "\xa0\x01", 13); break; case SYS_DVBC_ANNEX_A: - memcpy(cmd.args, "\x90\x01", 2); - cmd.wlen = 2; - cmd.rlen = 9; + CMD_SETUP(&cmd, "\x90\x01", 9); break; case SYS_DVBT2: - memcpy(cmd.args, "\x50\x01", 2); - cmd.wlen = 2; - cmd.rlen = 14; + CMD_SETUP(&cmd, "\x50\x01", 14); break; default: ret = -EINVAL; @@ -165,9 +167,7 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status) /* BER */ if (*status & FE_HAS_VITERBI) { - memcpy(cmd.args, "\x82\x00", 2); - cmd.wlen = 2; - cmd.rlen = 3; + CMD_SETUP(&cmd, "\x82\x00", 3); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -198,9 +198,7 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status) /* UCB */ if (*status & FE_HAS_SYNC) { - memcpy(cmd.args, "\x84\x01", 2); - cmd.wlen = 2; - cmd.rlen = 3; + CMD_SETUP(&cmd, "\x84\x01", 3); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -286,22 +284,18 @@ static int si2168_set_frontend(struct dvb_frontend *fe) goto err; } - memcpy(cmd.args, "\x88\x02\x02\x02\x02", 5); - cmd.wlen = 5; - cmd.rlen = 5; + CMD_SETUP(&cmd, "\x88\x02\x02\x02\x02", 5); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; /* that has no big effect */ if (c->delivery_system == SYS_DVBT) - memcpy(cmd.args, "\x89\x21\x06\x11\xff\x98", 6); + CMD_SETUP(&cmd, "\x89\x21\x06\x11\xff\x98", 3); else if (c->delivery_system == SYS_DVBC_ANNEX_A) - memcpy(cmd.args, "\x89\x21\x06\x11\x89\xf0", 6); + CMD_SETUP(&cmd, "\x89\x21\x06\x11\x89\xf0", 3); else if (c->delivery_system == SYS_DVBT2) - memcpy(cmd.args, "\x89\x21\x06\x11\x89\x20", 6); - cmd.wlen = 6; - cmd.rlen = 3; + CMD_SETUP(&cmd, "\x89\x21\x06\x11\x89\x20", 3); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -318,103 +312,77 @@ static int si2168_set_frontend(struct dvb_frontend *fe) goto err; } - memcpy(cmd.args, "\x51\x03", 2); - cmd.wlen = 2; - cmd.rlen = 12; + CMD_SETUP(&cmd, "\x51\x03", 12); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; - memcpy(cmd.args, "\x12\x08\x04", 3); - cmd.wlen = 3; - cmd.rlen = 3; + CMD_SETUP(&cmd, "\x12\x08\x04", 3); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; - memcpy(cmd.args, "\x14\x00\x0c\x10\x12\x00", 6); - cmd.wlen = 6; - cmd.rlen = 4; + CMD_SETUP(&cmd, "\x14\x00\x0c\x10\x12\x00", 4); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; - memcpy(cmd.args, "\x14\x00\x06\x10\x24\x00", 6); - cmd.wlen = 6; - cmd.rlen = 4; + CMD_SETUP(&cmd, "\x14\x00\x06\x10\x24\x00", 4); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; - memcpy(cmd.args, "\x14\x00\x07\x10\x00\x24", 6); - cmd.wlen = 6; - cmd.rlen = 4; + CMD_SETUP(&cmd, "\x14\x00\x07\x10\x00\x24", 4); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; - memcpy(cmd.args, "\x14\x00\x0a\x10\x00\x00", 6); + CMD_SETUP(&cmd, "\x14\x00\x0a\x10\x00\x00", 4); cmd.args[4] = delivery_system | bandwidth; if (dev->spectral_inversion) cmd.args[5] |= 1; - cmd.wlen = 6; - cmd.rlen = 4; ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; /* set DVB-C symbol rate */ if (c->delivery_system == SYS_DVBC_ANNEX_A) { - memcpy(cmd.args, "\x14\x00\x02\x11", 4); + CMD_SETUP(&cmd, "\x14\x00\x02\x11\x00\x00", 4); cmd.args[4] = ((c->symbol_rate / 1000) >> 0) & 0xff; cmd.args[5] = ((c->symbol_rate / 1000) >> 8) & 0xff; - cmd.wlen = 6; - cmd.rlen = 4; ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; } - memcpy(cmd.args, "\x14\x00\x0f\x10\x10\x00", 6); - cmd.wlen = 6; - cmd.rlen = 4; + CMD_SETUP(&cmd, "\x14\x00\x0f\x10\x10\x00", 4); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; - memcpy(cmd.args, "\x14\x00\x09\x10\xe3\x08", 6); + CMD_SETUP(&cmd, "\x14\x00\x09\x10\xe3\x08", 4); cmd.args[5] |= dev->ts_clock_inv ? 0x00 : 0x10; - cmd.wlen = 6; - cmd.rlen = 4; ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; - memcpy(cmd.args, "\x14\x00\x08\x10\xd7\x05", 6); + CMD_SETUP(&cmd, "\x14\x00\x08\x10\xd7\x05", 4); cmd.args[5] |= dev->ts_clock_inv ? 0x00 : 0x10; - cmd.wlen = 6; - cmd.rlen = 4; ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; - memcpy(cmd.args, "\x14\x00\x01\x12\x00\x00", 6); - cmd.wlen = 6; - cmd.rlen = 4; + CMD_SETUP(&cmd, "\x14\x00\x01\x12\x00\x00", 4); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; - memcpy(cmd.args, "\x14\x00\x01\x03\x0c\x00", 6); - cmd.wlen = 6; - cmd.rlen = 4; + CMD_SETUP(&cmd, "\x14\x00\x01\x03\x0c\x00", 4); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; - memcpy(cmd.args, "\x85", 1); - cmd.wlen = 1; - cmd.rlen = 1; + CMD_SETUP(&cmd, "\x85", 1); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -444,26 +412,20 @@ static int si2168_init(struct dvb_frontend *fe) dev_dbg(&client->dev, "\n"); /* initialize */ - memcpy(cmd.args, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13); - cmd.wlen = 13; - cmd.rlen = 0; + CMD_SETUP(&cmd, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 0); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; if (dev->warm) { /* resume */ - memcpy(cmd.args, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 8); - cmd.wlen = 8; - cmd.rlen = 1; + CMD_SETUP(&cmd, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 1); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; udelay(100); - memcpy(cmd.args, "\x85", 1); - cmd.wlen = 1; - cmd.rlen = 1; + CMD_SETUP(&cmd, "\x85", 1); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -472,9 +434,7 @@ static int si2168_init(struct dvb_frontend *fe) } /* power up */ - memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8); - cmd.wlen = 8; - cmd.rlen = 1; + CMD_SETUP(&cmd, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 1); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -542,17 +502,13 @@ static int si2168_init(struct dvb_frontend *fe) release_firmware(fw); - memcpy(cmd.args, "\x01\x01", 2); - cmd.wlen = 2; - cmd.rlen = 1; + CMD_SETUP(&cmd, "\x01\x01", 1); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; /* query firmware version */ - memcpy(cmd.args, "\x11", 1); - cmd.wlen = 1; - cmd.rlen = 10; + CMD_SETUP(&cmd, "\x11", 10); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -610,9 +566,7 @@ static int si2168_sleep(struct dvb_frontend *fe) if (dev->version > ('B' << 24 | 4 << 16 | 0 << 8 | 11 << 0)) dev->warm = false; - memcpy(cmd.args, "\x13", 1); - cmd.wlen = 1; - cmd.rlen = 0; + CMD_SETUP(&cmd, "\x13", 0); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -638,9 +592,7 @@ static int si2168_select(struct i2c_mux_core *muxc, u32 chan) struct si2168_cmd cmd; /* open I2C gate */ - memcpy(cmd.args, "\xc0\x0d\x01", 3); - cmd.wlen = 3; - cmd.rlen = 0; + CMD_SETUP(&cmd, "\xc0\x0d\x01", 0); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -658,9 +610,7 @@ static int si2168_deselect(struct i2c_mux_core *muxc, u32 chan) struct si2168_cmd cmd; /* close I2C gate */ - memcpy(cmd.args, "\xc0\x0d\x00", 3); - cmd.wlen = 3; - cmd.rlen = 0; + CMD_SETUP(&cmd, "\xc0\x0d\x00", 0); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -753,25 +703,19 @@ static int si2168_probe(struct i2c_client *client, mutex_init(&dev->i2c_mutex); /* Initialize */ - memcpy(cmd.args, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13); - cmd.wlen = 13; - cmd.rlen = 0; + CMD_SETUP(&cmd, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 0); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err_kfree; /* Power up */ - memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8); - cmd.wlen = 8; - cmd.rlen = 1; + CMD_SETUP(&cmd, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 1); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err_kfree; /* Query chip revision */ - memcpy(cmd.args, "\x02", 1); - cmd.wlen = 1; - cmd.rlen = 13; + CMD_SETUP(&cmd, "\x02", 13); ret = si2168_cmd_execute(client, &cmd); if (ret) goto err_kfree;