Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
> This driver should really have gotten some review before being included > in the kernel. > Even a simple checkpatch run finds more than > 250 stylistic errors > (not code bugs but cases where the driver violates the standard code > formatting rules of kernel code). Linus has strongly stated that we should merge hardware drivers early, and I agree: although the nes driver clearly needs more work, there's no advantage to users with the hardware in forcing them to wait for 2.6.26 to merge the driver, since they'll just have to patch the grungy code in themselves anyway. And by merging the driver early, we get fixed up for any tree-wide changes and allow janitors to help with the cleanup. (By the way, the code is not that pretty but it a lot closer to upstream style than most driver submissions) > And these are just comments from someone with zero knowledge about > InfiniBand, but I'd expect InfiniBand-specifig bugs might be found > before they hit users if an InfiniBand maintainer would review the > complete driver. Just for the record, although this driver is under drivers/infiniband, it is actually for a device that does iWARP/10 Gb ethernet. At some point we may want to rename drivers/infiniband to drivers/rdma, but so far the churn hasn't seemed worth it for what is basically a cosmetic issue. - R. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
On Thu, Feb 21, 2008 at 06:39:45AM -0600, Glenn Streiff wrote: > > > > > No, 51af33e8 was for a similar same bug 400 lines below > > this bug... > > > > > > Heh, sorry. > > > > > > Glenn -- please review Adrian's patches and let me know > > which ones are > > > good to apply. > > > > > > > I went ahead and created a patch series and attributed Adrian > for the patches of his I liked. There were a couple that > I tweaked. Wasn't sure if all the hunks would apply nicely > after that if we mixed and matched his and mine, hence the series. > > Hope that's okay. Should I have gotten his ack for the ones > I rewrote? The fixes were pretty small so I figured they didn't > really need more review. >... Looking at the patches what you did seems OK. But regarding "review" I have a different criticism directed at Roland: This driver should really have gotten some review before being included in the kernel. Even a simple checkpatch run finds more than > 250 stylistic errors (not code bugs but cases where the driver violates the standard code formatting rules of kernel code). And I'm not talking about the > 2000 checkpatch warnings that are mostly about too long lines (which should arguably also be fixed). And many more issues that could have been foung during a review. E.g. when you look at 3/8 from this series the code if (!cm_node) return -EINVAL; new_send = kzalloc(sizeof(*new_send), GFP_ATOMIC); if (!new_send) return -1; doesn't look good since the -1 should most likely better be something like -ENOMEM (I haven't checked whether you can immediately change it at this specific place). And these are just comments from someone with zero knowledge about InfiniBand, but I'd expect InfiniBand-specifig bugs might be found before they hit users if an InfiniBand maintainer would review the complete driver. Note that this is not meant as a criticism against Glenn - it's normal that submitted code contains bugs, but a code review can help to cope with this. > Glenn cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
> > > No, 51af33e8 was for a similar same bug 400 lines below > this bug... > > > > Heh, sorry. > > > > Glenn -- please review Adrian's patches and let me know > which ones are > > good to apply. > > > I went ahead and created a patch series and attributed Adrian for the patches of his I liked. There were a couple that I tweaked. Wasn't sure if all the hunks would apply nicely after that if we mixed and matched his and mine, hence the series. Hope that's okay. Should I have gotten his ack for the ones I rewrote? The fixes were pretty small so I figured they didn't really need more review. The patch series is on the way... Glenn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
No, 51af33e8 was for a similar same bug 400 lines below this bug... Heh, sorry. Glenn -- please review Adrian's patches and let me know which ones are good to apply. I went ahead and created a patch series and attributed Adrian for the patches of his I liked. There were a couple that I tweaked. Wasn't sure if all the hunks would apply nicely after that if we mixed and matched his and mine, hence the series. Hope that's okay. Should I have gotten his ack for the ones I rewrote? The fixes were pretty small so I figured they didn't really need more review. The patch series is on the way... Glenn -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
On Thu, Feb 21, 2008 at 06:39:45AM -0600, Glenn Streiff wrote: No, 51af33e8 was for a similar same bug 400 lines below this bug... Heh, sorry. Glenn -- please review Adrian's patches and let me know which ones are good to apply. I went ahead and created a patch series and attributed Adrian for the patches of his I liked. There were a couple that I tweaked. Wasn't sure if all the hunks would apply nicely after that if we mixed and matched his and mine, hence the series. Hope that's okay. Should I have gotten his ack for the ones I rewrote? The fixes were pretty small so I figured they didn't really need more review. ... Looking at the patches what you did seems OK. But regarding review I have a different criticism directed at Roland: This driver should really have gotten some review before being included in the kernel. Even a simple checkpatch run finds more than 250 stylistic errors (not code bugs but cases where the driver violates the standard code formatting rules of kernel code). And I'm not talking about the 2000 checkpatch warnings that are mostly about too long lines (which should arguably also be fixed). And many more issues that could have been foung during a review. E.g. when you look at 3/8 from this series the code if (!cm_node) return -EINVAL; new_send = kzalloc(sizeof(*new_send), GFP_ATOMIC); if (!new_send) return -1; doesn't look good since the -1 should most likely better be something like -ENOMEM (I haven't checked whether you can immediately change it at this specific place). And these are just comments from someone with zero knowledge about InfiniBand, but I'd expect InfiniBand-specifig bugs might be found before they hit users if an InfiniBand maintainer would review the complete driver. Note that this is not meant as a criticism against Glenn - it's normal that submitted code contains bugs, but a code review can help to cope with this. Glenn cu Adrian -- Is there not promise of rain? Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. Only a promise, Lao Er said. Pearl S. Buck - Dragon Seed -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
This driver should really have gotten some review before being included in the kernel. Even a simple checkpatch run finds more than 250 stylistic errors (not code bugs but cases where the driver violates the standard code formatting rules of kernel code). Linus has strongly stated that we should merge hardware drivers early, and I agree: although the nes driver clearly needs more work, there's no advantage to users with the hardware in forcing them to wait for 2.6.26 to merge the driver, since they'll just have to patch the grungy code in themselves anyway. And by merging the driver early, we get fixed up for any tree-wide changes and allow janitors to help with the cleanup. (By the way, the code is not that pretty but it a lot closer to upstream style than most driver submissions) And these are just comments from someone with zero knowledge about InfiniBand, but I'd expect InfiniBand-specifig bugs might be found before they hit users if an InfiniBand maintainer would review the complete driver. Just for the record, although this driver is under drivers/infiniband, it is actually for a device that does iWARP/10 Gb ethernet. At some point we may want to rename drivers/infiniband to drivers/rdma, but so far the churn hasn't seemed worth it for what is basically a cosmetic issue. - R. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
> -Original Message- > From: Roland Dreier [mailto:[EMAIL PROTECTED] > Sent: Wednesday, February 20, 2008 5:22 PM > To: Adrian Bunk > Cc: Faisal Latif; Glenn Streiff; linux-kernel@vger.kernel.org; > [EMAIL PROTECTED] > Subject: Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: > fix off-by-one > > > > No, 51af33e8 was for a similar same bug 400 lines below this bug... > > Heh, sorry. > > Glenn -- please review Adrian's patches and let me know which ones are > good to apply. > Sweeping through them right now. Should have something for you tonight. Glenn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
> No, 51af33e8 was for a similar same bug 400 lines below this bug... Heh, sorry. Glenn -- please review Adrian's patches and let me know which ones are good to apply. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
No, 51af33e8 was for a similar same bug 400 lines below this bug... Heh, sorry. Glenn -- please review Adrian's patches and let me know which ones are good to apply. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
-Original Message- From: Roland Dreier [mailto:[EMAIL PROTECTED] Sent: Wednesday, February 20, 2008 5:22 PM To: Adrian Bunk Cc: Faisal Latif; Glenn Streiff; linux-kernel@vger.kernel.org; [EMAIL PROTECTED] Subject: Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one No, 51af33e8 was for a similar same bug 400 lines below this bug... Heh, sorry. Glenn -- please review Adrian's patches and let me know which ones are good to apply. Sweeping through them right now. Should have something for you tonight. Glenn -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
On Tue, Feb 19, 2008 at 08:23:19PM -0800, Roland Dreier wrote: > Thanks, this is already upstream as 51af33e8 No, 51af33e8 was for a similar same bug 400 lines below this bug... cu Adrian -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
Thanks, this is already upstream as 51af33e8 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
Thanks, this is already upstream as 51af33e8 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] [2.6 patch] infiniband/hw/nes/nes_verbs.c: fix off-by-one
On Tue, Feb 19, 2008 at 08:23:19PM -0800, Roland Dreier wrote: Thanks, this is already upstream as 51af33e8 No, 51af33e8 was for a similar same bug 400 lines below this bug... cu Adrian -- Is there not promise of rain? Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. Only a promise, Lao Er said. Pearl S. Buck - Dragon Seed -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
