Messages in this thread | | | Date | Tue, 14 Dec 2021 03:44:19 +0100 | From | rkardell <> | Subject | About patches |
| |
I explicit asked if the patches were ok when I sent them and I got no response. Now I received this “This patch is broken on so many ways:” with comments. Responses like this will certainly not encourage new members, especially since the comments and changes of the code was not of high quality. And it certainly discourage me!
I include a copy of my comments sent to the responsible person and de-register from the group.
Den 2021-12-07 kl. 09:29, skrev rkardell: > > Sorry if I created problems for linux-media by pointing to a solution to a problem that has been in the kernel since the introduction of USB EHCI. The only PC I have that works with this DVB device is from 2005. > > I first asked for comments since C/C++ is not my preferred language and Linux kernel is not my normal working environment, but I got no response. I submitted the patches to the best of my knowledge. > > > 1. Your documented way of submitting patches did not work, so I had to retrieve the patch and send it as an ordinary mail. > > 2. If you cant get 1 byte buffer in the kernel, then you wont reach this code, but yes I could have tested. > > 3. I didn’t change the write code, because it was not a problem. > > 4. Sorry for that > > It is obvious that its the USB transfer that is the problem since USB is used for the i2c transfer! > > I have no way of knowing what combination of chips/drivers are used and possible side effects of changes. That’s way I preferred to change the the code this way > > From the documentation I found, it’s not the use of pointers on the stack that is the problem, its the use of buffer on the stack. > > > About your code: you don’t have to free memory that you could not allocate. > > My pc is updated with my patch, so I cant test your solution. > > > I don’t need this type of comment after my effort to point out the problem that has been in the kernel for such a long time, so I will de-register from the group. > > > > Den 2021-12-06 kl. 16:01, skrev Mauro Carvalho Chehab: >> Hi, >> >> This patch is broken on so many ways: >> >> 1. your e-mailer converted spaces and tabs into UTF spaces (0xa0); >> 2. it is not checking if allocation failed; >> 3. it doesn't fix the write function, which also uses stack; >> 4. you need your real name on your from and SoB. >> >> Besides that, there's no issue on using the stack for I2C transfers. >> The issue, is actually to use stack for USB transfers. >> >> So, the right fix should be, instead, at the bridge driver, and not >> on tuner. It seems that Mega Sky 580 USB DVB is supportd by m920x driver. >> Right? >> >> If so, the enclosed patch should fix the issue. >> >> Please test. >> >> Regards, >> Mauro >> >> Em Thu, 7 Oct 2021 14:29:00 +0200 >> rkardell <rkardell@mida.se> escreveu: >> >>> Solve problem with initialization of Mega Sky 580 USB DVB (and other >>> using mt352), error when reading i2c id. >>> >>> >>> Signed-off-by: rkl099 <rkardell@mida.se> >>> --- >>> drivers/media/tuners/qt1010.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/tuners/qt1010.c b/drivers/media/tuners/qt1010.c >>> index 3853a3d43..1bc0756f7 100644 >>> --- a/drivers/media/tuners/qt1010.c >>> +++ b/drivers/media/tuners/qt1010.c >>> @@ -11,18 +11,22 @@ >>> /* read single register */ >>> static int qt1010_readreg(struct qt1010_priv *priv, u8 reg, u8 *val) >>> { >>> + u8 *b1=kmalloc(1,GFP_KERNEL); >>> struct i2c_msg msg[2] = { >>> { .addr = priv->cfg->i2c_address, >>> .flags = 0, .buf = ®, .len = 1 }, >>> { .addr = priv->cfg->i2c_address, >>> - .flags = I2C_M_RD, .buf = val, .len = 1 }, >>> + .flags = I2C_M_RD, .buf = b1, .len = 1 }, >>> }; >>> >>> if (i2c_transfer(priv->i2c, msg, 2) != 2) { >>> dev_warn(&priv->i2c->dev, "%s: i2c rd failed reg=%02x\n", >>> KBUILD_MODNAME, reg); >>> + kfree(b1); >>> return -EREMOTEIO; >>> } >>> + *val=b1[0]; >>> + kfree(b1); >>> return 0; >>> } >>> >> Thanks, >> Mauro >> >> [PATCH] media: m920x: don't use stack on USB reads >> >> Using stack-allocated pointers for USB message data don't work. >> This driver is almost OK with that, except for the I2C read >> logic. >> >> Fix it by using a temporary read buffer, just like on all other >> calls to m920x_read(). >> >> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> >> >> diff --git a/drivers/media/usb/dvb-usb/m920x.c b/drivers/media/usb/dvb-usb/m920x.c >> index 4bb5b82599a7..691e05833db1 100644 >> --- a/drivers/media/usb/dvb-usb/m920x.c >> +++ b/drivers/media/usb/dvb-usb/m920x.c >> @@ -274,6 +274,13 @@ static int m920x_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], int nu >> /* Should check for ack here, if we knew how. */ >> } >> if (msg[i].flags & I2C_M_RD) { >> + char *read = kmalloc(1, GFP_KERNEL); >> + if (!read) { >> + ret = -ENOMEM; >> + kfree(read); >> + goto unlock; >> + } >> + >> for (j = 0; j < msg[i].len; j++) { >> /* Last byte of transaction? >> * Send STOP, otherwise send ACK. */ >> @@ -281,9 +288,12 @@ static int m920x_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msg[], int nu >> >> if ((ret = m920x_read(d->udev, M9206_I2C, 0x0, >> 0x20 | stop, >> - &msg[i].buf[j], 1)) != 0) >> + read, 1)) != 0) >> goto unlock; >> + msg[i].buf[j] = read[0]; >> } >> + >> + kfree(read); >> } else { >> for (j = 0; j < msg[i].len; j++) { >> /* Last byte of transaction? Then send STOP. */ >> >
| |