| Message ID | 1ab6b168-3c69-97c2-d02e-cd64b7fa222f@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
This patch introduces bugs. It's my policy to not explain the Markus's bugs because otherwise he will just resend the patchset and I have asked him many times to stop. I will happily review bug fix patches but I really think he should stop sending these cleanup patches. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24/10/16 23:01, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Mon, 24 Oct 2016 22:44:02 +0200 > > Move the assignment for the data structure members "isoc_copy" > and "num_bufs" behind the source code for memory allocations > by this function. I don't see the point, dropping this patch. Hans > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/media/usb/au0828/au0828-video.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c > index b5c88a7..5ebda64 100644 > --- a/drivers/media/usb/au0828/au0828-video.c > +++ b/drivers/media/usb/au0828/au0828-video.c > @@ -218,9 +218,6 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, > int rc; > > au0828_isocdbg("au0828: called au0828_prepare_isoc\n"); > - > - dev->isoc_ctl.isoc_copy = isoc_copy; > - dev->isoc_ctl.num_bufs = num_bufs; > dev->isoc_ctl.urb = kcalloc(num_bufs, > sizeof(*dev->isoc_ctl.urb), > GFP_KERNEL); > @@ -240,6 +237,7 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, > dev->isoc_ctl.buf = NULL; > > sb_size = max_packets * dev->isoc_ctl.max_pkt_size; > + dev->isoc_ctl.num_bufs = num_bufs; > > /* allocate urbs and transfer buffers */ > for (i = 0; i < dev->isoc_ctl.num_bufs; i++) { > @@ -276,6 +274,7 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, > k += dev->isoc_ctl.max_pkt_size; > } > } > + dev->isoc_ctl.isoc_copy = isoc_copy; > > /* submit urbs and enables IRQ */ > for (i = 0; i < dev->isoc_ctl.num_bufs; i++) { > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> From: Markus Elfring <elfring@users.sourceforge.net> >> Date: Mon, 24 Oct 2016 22:44:02 +0200 >> >> Move the assignment for the data structure members "isoc_copy" >> and "num_bufs" behind the source code for memory allocations >> by this function. > > I don't see the point, Another explanation try … > dropping this patch. I proposed that these assignments should only be performed after the required memory allocations succeeded. Is this detail worth for further considerations? >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >> --- >> drivers/media/usb/au0828/au0828-video.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c >> index b5c88a7..5ebda64 100644 >> --- a/drivers/media/usb/au0828/au0828-video.c >> +++ b/drivers/media/usb/au0828/au0828-video.c >> @@ -218,9 +218,6 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, >> int rc; >> >> au0828_isocdbg("au0828: called au0828_prepare_isoc\n"); >> - >> - dev->isoc_ctl.isoc_copy = isoc_copy; >> - dev->isoc_ctl.num_bufs = num_bufs; >> dev->isoc_ctl.urb = kcalloc(num_bufs, >> sizeof(*dev->isoc_ctl.urb), >> GFP_KERNEL); >> @@ -240,6 +237,7 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, >> dev->isoc_ctl.buf = NULL; >> >> sb_size = max_packets * dev->isoc_ctl.max_pkt_size; >> + dev->isoc_ctl.num_bufs = num_bufs; >> >> /* allocate urbs and transfer buffers */ >> for (i = 0; i < dev->isoc_ctl.num_bufs; i++) { >> @@ -276,6 +274,7 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, >> k += dev->isoc_ctl.max_pkt_size; >> } >> } >> + dev->isoc_ctl.isoc_copy = isoc_copy; >> >> /* submit urbs and enables IRQ */ >> for (i = 0; i < dev->isoc_ctl.num_bufs; i++) { >> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/11/16 20:56, SF Markus Elfring wrote: >>> From: Markus Elfring <elfring@users.sourceforge.net> >>> Date: Mon, 24 Oct 2016 22:44:02 +0200 >>> >>> Move the assignment for the data structure members "isoc_copy" >>> and "num_bufs" behind the source code for memory allocations >>> by this function. >> >> I don't see the point, > > Another explanation try … > > >> dropping this patch. > > I proposed that these assignments should only be performed after the required > memory allocations succeeded. Is this detail worth for further considerations? I understand why you think it is better, but I disagree :-) I prefer the current approach, that way I know as a reviewer that these fields are correctly set and I can forget about them. Not worth spending more time on this. Regards, Hans > > >>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >>> --- >>> drivers/media/usb/au0828/au0828-video.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c >>> index b5c88a7..5ebda64 100644 >>> --- a/drivers/media/usb/au0828/au0828-video.c >>> +++ b/drivers/media/usb/au0828/au0828-video.c >>> @@ -218,9 +218,6 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, >>> int rc; >>> >>> au0828_isocdbg("au0828: called au0828_prepare_isoc\n"); >>> - >>> - dev->isoc_ctl.isoc_copy = isoc_copy; >>> - dev->isoc_ctl.num_bufs = num_bufs; >>> dev->isoc_ctl.urb = kcalloc(num_bufs, >>> sizeof(*dev->isoc_ctl.urb), >>> GFP_KERNEL); >>> @@ -240,6 +237,7 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, >>> dev->isoc_ctl.buf = NULL; >>> >>> sb_size = max_packets * dev->isoc_ctl.max_pkt_size; >>> + dev->isoc_ctl.num_bufs = num_bufs; >>> >>> /* allocate urbs and transfer buffers */ >>> for (i = 0; i < dev->isoc_ctl.num_bufs; i++) { >>> @@ -276,6 +274,7 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, >>> k += dev->isoc_ctl.max_pkt_size; >>> } >>> } >>> + dev->isoc_ctl.isoc_copy = isoc_copy; >>> >>> /* submit urbs and enables IRQ */ >>> for (i = 0; i < dev->isoc_ctl.num_bufs; i++) { >>> > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c index b5c88a7..5ebda64 100644 --- a/drivers/media/usb/au0828/au0828-video.c +++ b/drivers/media/usb/au0828/au0828-video.c @@ -218,9 +218,6 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, int rc; au0828_isocdbg("au0828: called au0828_prepare_isoc\n"); - - dev->isoc_ctl.isoc_copy = isoc_copy; - dev->isoc_ctl.num_bufs = num_bufs; dev->isoc_ctl.urb = kcalloc(num_bufs, sizeof(*dev->isoc_ctl.urb), GFP_KERNEL); @@ -240,6 +237,7 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, dev->isoc_ctl.buf = NULL; sb_size = max_packets * dev->isoc_ctl.max_pkt_size; + dev->isoc_ctl.num_bufs = num_bufs; /* allocate urbs and transfer buffers */ for (i = 0; i < dev->isoc_ctl.num_bufs; i++) { @@ -276,6 +274,7 @@ static int au0828_init_isoc(struct au0828_dev *dev, int max_packets, k += dev->isoc_ctl.max_pkt_size; } } + dev->isoc_ctl.isoc_copy = isoc_copy; /* submit urbs and enables IRQ */ for (i = 0; i < dev->isoc_ctl.num_bufs; i++) {