| Message ID | 200912032256.39790.bzolnier@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 12/03/2009 04:56 PM, Bartlomiej Zolnierkiewicz wrote: > On Thursday 03 December 2009 10:51:09 pm Jeff Garzik wrote: > >>>>> pata_via: clear UDMA transfer mode bit for PIO and MWDMA >>>> >>>> applied -- even though Alan's comment was correct. It is standard >>>> kernel practice to place cosmetic changes into their own patches, >>>> because it is standard kernel practice to break up logically distinct >>>> changes. >>> >>> We are talking about: >>> >>> pata_via.c | 19 +++++++++++++------ >>> 1 file changed, 13 insertions(+), 6 deletions(-) >>> >>> patch here (http://lkml.org/lkml/2009/11/25/380) and cosmetic change >>> is clearly documented in the patch description. >>> >>> >>> Do people really wonder why I find upstream to be too much hassle to >>> deal with? >> >> The thousand other kernel developers seem to be able to split up their >> patches, separating out cosmetic changes from functional ones. It has >> clear engineering benefits, and has been standard practice for a decade >> or more. >> >> Why is it such an imposition for your patches to look like everyone >> else's? And by "everyone", I mean all other kernel developers, not just >> other ATA developers. >> >> You seem to consider standard kernel practice a hassle. Separating out >> cosmetic changes is not only a libata practice, it is the norm for the >> entire kernel. > > Indeed. > > From 94be9a58d7e683ac3c1df1858a17f09ebade8da0 Mon Sep 17 00:00:00 2001 > From: Jeff Garzik<jeff@garzik.org> > Date: Fri, 16 Jan 2009 10:17:09 -0500 > Subject: [PATCH] [libata] get-identity ioctl: Fix use of invalid memory pointer > for SAS drivers. > > Caught by Ke Wei (and team?) at Marvell. > > Also, move the ata_scsi_ioctl export to libata-scsi.c, as that seems to be the > general trend. > > Acked-by: James Bottomley<James.Bottomley@HansenPartnership.com> > Signed-off-by: Jeff Garzik<jgarzik@redhat.com> If your point, by posting this patch, is that it includes a ton of gratuitous cosmetic changes, you misread the patch. ata_scsi_ioctl() remains in existence; only the callers need to use the new SAS-related ioctl function were updated. The remainder continued to use ata_scsi_ioctl(). Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 03 December 2009 11:02:36 pm Jeff Garzik wrote: > On 12/03/2009 04:56 PM, Bartlomiej Zolnierkiewicz wrote: > > On Thursday 03 December 2009 10:51:09 pm Jeff Garzik wrote: > > > >>>>> pata_via: clear UDMA transfer mode bit for PIO and MWDMA > >>>> > >>>> applied -- even though Alan's comment was correct. It is standard > >>>> kernel practice to place cosmetic changes into their own patches, > >>>> because it is standard kernel practice to break up logically distinct > >>>> changes. > >>> > >>> We are talking about: > >>> > >>> pata_via.c | 19 +++++++++++++------ > >>> 1 file changed, 13 insertions(+), 6 deletions(-) > >>> > >>> patch here (http://lkml.org/lkml/2009/11/25/380) and cosmetic change > >>> is clearly documented in the patch description. > >>> > >>> > >>> Do people really wonder why I find upstream to be too much hassle to > >>> deal with? > >> > >> The thousand other kernel developers seem to be able to split up their > >> patches, separating out cosmetic changes from functional ones. It has > >> clear engineering benefits, and has been standard practice for a decade > >> or more. > >> > >> Why is it such an imposition for your patches to look like everyone > >> else's? And by "everyone", I mean all other kernel developers, not just > >> other ATA developers. > >> > >> You seem to consider standard kernel practice a hassle. Separating out > >> cosmetic changes is not only a libata practice, it is the norm for the > >> entire kernel. > > > > Indeed. > > > > From 94be9a58d7e683ac3c1df1858a17f09ebade8da0 Mon Sep 17 00:00:00 2001 > > From: Jeff Garzik<jeff@garzik.org> > > Date: Fri, 16 Jan 2009 10:17:09 -0500 > > Subject: [PATCH] [libata] get-identity ioctl: Fix use of invalid memory pointer > > for SAS drivers. > > > > Caught by Ke Wei (and team?) at Marvell. > > > > Also, move the ata_scsi_ioctl export to libata-scsi.c, as that seems to be the > > general trend. > > > > Acked-by: James Bottomley<James.Bottomley@HansenPartnership.com> > > Signed-off-by: Jeff Garzik<jgarzik@redhat.com> > > If your point, by posting this patch, is that it includes a ton of > gratuitous cosmetic changes, you misread the patch. > > ata_scsi_ioctl() remains in existence; only the callers need to use the > new SAS-related ioctl function were updated. The remainder continued to > use ata_scsi_ioctl(). Moving kernel exports around is completely unrelated to a bug fix. Yes, it is convenient to do it in the same patch and OK with me. -- Bartlomiej Zolnierkiewicz -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/03/2009 05:06 PM, Bartlomiej Zolnierkiewicz wrote: > On Thursday 03 December 2009 11:02:36 pm Jeff Garzik wrote: >> On 12/03/2009 04:56 PM, Bartlomiej Zolnierkiewicz wrote: >>> On Thursday 03 December 2009 10:51:09 pm Jeff Garzik wrote: >>> >>>>>>> pata_via: clear UDMA transfer mode bit for PIO and MWDMA >>>>>> >>>>>> applied -- even though Alan's comment was correct. It is standard >>>>>> kernel practice to place cosmetic changes into their own patches, >>>>>> because it is standard kernel practice to break up logically distinct >>>>>> changes. >>>>> >>>>> We are talking about: >>>>> >>>>> pata_via.c | 19 +++++++++++++------ >>>>> 1 file changed, 13 insertions(+), 6 deletions(-) >>>>> >>>>> patch here (http://lkml.org/lkml/2009/11/25/380) and cosmetic change >>>>> is clearly documented in the patch description. >>>>> >>>>> >>>>> Do people really wonder why I find upstream to be too much hassle to >>>>> deal with? >>>> >>>> The thousand other kernel developers seem to be able to split up their >>>> patches, separating out cosmetic changes from functional ones. It has >>>> clear engineering benefits, and has been standard practice for a decade >>>> or more. >>>> >>>> Why is it such an imposition for your patches to look like everyone >>>> else's? And by "everyone", I mean all other kernel developers, not just >>>> other ATA developers. >>>> >>>> You seem to consider standard kernel practice a hassle. Separating out >>>> cosmetic changes is not only a libata practice, it is the norm for the >>>> entire kernel. >>> >>> Indeed. >>> >>> From 94be9a58d7e683ac3c1df1858a17f09ebade8da0 Mon Sep 17 00:00:00 2001 >>> From: Jeff Garzik<jeff@garzik.org> >>> Date: Fri, 16 Jan 2009 10:17:09 -0500 >>> Subject: [PATCH] [libata] get-identity ioctl: Fix use of invalid memory pointer >>> for SAS drivers. >>> >>> Caught by Ke Wei (and team?) at Marvell. >>> >>> Also, move the ata_scsi_ioctl export to libata-scsi.c, as that seems to be the >>> general trend. >>> >>> Acked-by: James Bottomley<James.Bottomley@HansenPartnership.com> >>> Signed-off-by: Jeff Garzik<jgarzik@redhat.com> >> >> If your point, by posting this patch, is that it includes a ton of >> gratuitous cosmetic changes, you misread the patch. >> >> ata_scsi_ioctl() remains in existence; only the callers need to use the >> new SAS-related ioctl function were updated. The remainder continued to >> use ata_scsi_ioctl(). > > Moving kernel exports around is completely unrelated to a bug fix. Did the patch contain -cosmetic- changes intermingled with code changes, in the same code lines? No. Is it good kernel practice to intermingle cosmetic changes with functional ones, in the same code lines? Also, no. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/03/2009 05:10 PM, Jeff Garzik wrote: > Did the patch contain -cosmetic- changes intermingled with code changes, > in the same code lines? No. > > Is it good kernel practice to intermingle cosmetic changes with > functional ones, in the same code lines? Also, no. In fact, I recall one case where a certain developer on this list used cosmetic code changes (Lindent, IIRC) to hide functional, security-related code changes. Mixing cosmetic and function changes is simply bad engineering practice, _especially_ when they occur in the same lines of code. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" 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/ata/libata-core.c b/drivers/ata/libata-core.c index 71218d7..552ecae 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -6638,7 +6638,6 @@ EXPORT_SYMBOL_GPL(ata_dev_pair); EXPORT_SYMBOL_GPL(ata_port_disable); EXPORT_SYMBOL_GPL(ata_ratelimit); EXPORT_SYMBOL_GPL(ata_wait_register); -EXPORT_SYMBOL_GPL(ata_scsi_ioctl); EXPORT_SYMBOL_GPL(ata_scsi_queuecmd); EXPORT_SYMBOL_GPL(ata_scsi_slave_config); EXPORT_SYMBOL_GPL(ata_scsi_slave_destroy); diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 9e92107..a1a6e62 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -423,9 +423,9 @@ int ata_std_bios_param(struct scsi_device *sdev, struct block_device *bdev, * RETURNS: * Zero on success, negative errno on error. */ -static int ata_get_identity(struct scsi_device *sdev, void __user *arg) +static int ata_get_identity(struct ata_port *ap, struct scsi_device *sdev, + void __user *arg) { - struct ata_port *ap = ata_shost_to_port(sdev->host); struct ata_device *dev = ata_scsi_find_dev(ap, sdev); u16 __user *dst = arg; char buf[40]; @@ -645,7 +645,8 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg) return rc; } -int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg) +int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *scsidev, + int cmd, void __user *arg) { int val = -EINVAL, rc = -EINVAL; @@ -663,7 +664,7 @@ int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg) return 0; case HDIO_GET_IDENTITY: - return ata_get_identity(scsidev, arg); + return ata_get_identity(ap, scsidev, arg); case HDIO_DRIVE_CMD: if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO)) @@ -682,6 +683,14 @@ int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg) return rc; } +EXPORT_SYMBOL_GPL(ata_sas_scsi_ioctl); + +int ata_scsi_ioctl(struct scsi_device *scsidev, int cmd, void __user *arg) +{ + return ata_sas_scsi_ioctl(ata_shost_to_port(scsidev->host), + scsidev, cmd, arg); +} +EXPORT_SYMBOL_GPL(ata_scsi_ioctl); /** * ata_scsi_qc_new - acquire new ata_queued_cmd reference diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c index 841f460..0782900 100644 --- a/drivers/scsi/ipr.c +++ b/drivers/scsi/ipr.c @@ -4912,7 +4912,7 @@ static int ipr_ioctl(struct scsi_device *sdev, int cmd, void __user *arg) if (res && ipr_is_gata(res)) { if (cmd == HDIO_GET_IDENTITY) return -ENOTTY; - return ata_scsi_ioctl(sdev, cmd, arg); + return ata_sas_scsi_ioctl(res->sata_port->ap, sdev, cmd, arg); } return -EINVAL; diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index 7448387..1c558d3 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -717,7 +717,7 @@ int sas_ioctl(struct scsi_device *sdev, int cmd, void __user *arg) struct domain_device *dev = sdev_to_domain_dev(sdev); if (dev_is_sata(dev)) - return ata_scsi_ioctl(sdev, cmd, arg); + return ata_sas_scsi_ioctl(dev->sata_dev.ap, sdev, cmd, arg); return -EINVAL; } diff --git a/include/linux/libata.h b/include/linux/libata.h index b6b8a7f..73b69c7 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -927,6 +927,8 @@ extern void ata_host_init(struct ata_host *, struct device *, extern int ata_scsi_detect(struct scsi_host_template *sht); extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg); extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *)); +extern int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *dev, + int cmd, void __user *arg); extern void ata_sas_port_destroy(struct ata_port *); extern struct ata_port *ata_sas_port_alloc(struct ata_host *, struct ata_port_info *, struct Scsi_Host *);