| Message ID | 20180504150620.6501-1-bigeasy@linutronix.de |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | libata: remove ata_sff_data_xfer_noirq() | expand |
Hello, Sebastian. On Fri, May 04, 2018 at 05:06:20PM +0200, Sebastian Andrzej Siewior wrote: > ata_sff_data_xfer_noirq() is invoked via the ->sff_data_xfer hook. The > latter is invoked by ata_pio_sector(), atapi_send_cdb() and > __atapi_pio_bytes() which in turn is invoked by ata_sff_hsm_move(). > The latter function requires that the "ap->lock" lock is held which > needs to be taken with disabled interrupts. > > There is no need have to have ata_sff_data_xfer_noirq() which invokes > ata_sff_data_xfer32() with disabled interrupts because at this point the > interrupts are already disabled. > Remove the function and its references to it and replace all callers > with ata_sff_data_xfer32(). Can you please add irq disabled assert to ata_sff_data_xfer*()? Thanks.
On 2018-05-07 08:49:08 [-0700], Tejun Heo wrote: > Hello, Sebastian. > > On Fri, May 04, 2018 at 05:06:20PM +0200, Sebastian Andrzej Siewior wrote: > > ata_sff_data_xfer_noirq() is invoked via the ->sff_data_xfer hook. The > > latter is invoked by ata_pio_sector(), atapi_send_cdb() and > > __atapi_pio_bytes() which in turn is invoked by ata_sff_hsm_move(). > > The latter function requires that the "ap->lock" lock is held which > > needs to be taken with disabled interrupts. > > > > There is no need have to have ata_sff_data_xfer_noirq() which invokes > > ata_sff_data_xfer32() with disabled interrupts because at this point the > > interrupts are already disabled. > > Remove the function and its references to it and replace all callers > > with ata_sff_data_xfer32(). > > Can you please add irq disabled assert to ata_sff_data_xfer*()? Why irq-disabled assert? Can we use lockdep_assert_held() instead? > Thanks. Sebastian -- 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 2018-05-07 17:52:16 [+0200], To Tejun Heo wrote: > On 2018-05-07 08:49:08 [-0700], Tejun Heo wrote: > > Hello, Sebastian. Hi Tejun, > > On Fri, May 04, 2018 at 05:06:20PM +0200, Sebastian Andrzej Siewior wrote: > > > ata_sff_data_xfer_noirq() is invoked via the ->sff_data_xfer hook. The > > > latter is invoked by ata_pio_sector(), atapi_send_cdb() and > > > __atapi_pio_bytes() which in turn is invoked by ata_sff_hsm_move(). > > > The latter function requires that the "ap->lock" lock is held which > > > needs to be taken with disabled interrupts. > > > > > > There is no need have to have ata_sff_data_xfer_noirq() which invokes > > > ata_sff_data_xfer32() with disabled interrupts because at this point the > > > interrupts are already disabled. > > > Remove the function and its references to it and replace all callers > > > with ata_sff_data_xfer32(). > > > > Can you please add irq disabled assert to ata_sff_data_xfer*()? > > Why irq-disabled assert? Can we use lockdep_assert_held() instead? That irq-disabled assert won't work on RT as expected that is why I intend to remove the local_irq_save() (which is not needed). If we could avoid the irq-disabled assert or use a lock instead, then it wouldn't trigger another error on RT. > > Thanks. Sebastian -- 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/Documentation/driver-api/libata.rst b/Documentation/driver-api/libata.rst index 4adc056f7635..70e180e6b93d 100644 --- a/Documentation/driver-api/libata.rst +++ b/Documentation/driver-api/libata.rst @@ -118,8 +118,7 @@ PIO data read/write All bmdma-style drivers must implement this hook. This is the low-level operation that actually copies the data bytes during a PIO data transfer. Typically the driver will choose one of -:c:func:`ata_sff_data_xfer_noirq`, :c:func:`ata_sff_data_xfer`, or -:c:func:`ata_sff_data_xfer32`. +:c:func:`ata_sff_data_xfer`, or :c:func:`ata_sff_data_xfer32`. ATA command execute ~~~~~~~~~~~~~~~~~~~ diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c index cc2f2e35f4c2..c5ea0fc635e5 100644 --- a/drivers/ata/libata-sff.c +++ b/drivers/ata/libata-sff.c @@ -657,36 +657,6 @@ unsigned int ata_sff_data_xfer32(struct ata_queued_cmd *qc, unsigned char *buf, } EXPORT_SYMBOL_GPL(ata_sff_data_xfer32); -/** - * ata_sff_data_xfer_noirq - Transfer data by PIO - * @qc: queued command - * @buf: data buffer - * @buflen: buffer length - * @rw: read/write - * - * Transfer data from/to the device data register by PIO. Do the - * transfer with interrupts disabled. - * - * LOCKING: - * Inherited from caller. - * - * RETURNS: - * Bytes consumed. - */ -unsigned int ata_sff_data_xfer_noirq(struct ata_queued_cmd *qc, unsigned char *buf, - unsigned int buflen, int rw) -{ - unsigned long flags; - unsigned int consumed; - - local_irq_save(flags); - consumed = ata_sff_data_xfer32(qc, buf, buflen, rw); - local_irq_restore(flags); - - return consumed; -} -EXPORT_SYMBOL_GPL(ata_sff_data_xfer_noirq); - /** * ata_pio_sector - Transfer a sector of data. * @qc: Command on going diff --git a/drivers/ata/pata_cmd640.c b/drivers/ata/pata_cmd640.c index c47caa807fa9..e3532eda7b05 100644 --- a/drivers/ata/pata_cmd640.c +++ b/drivers/ata/pata_cmd640.c @@ -178,7 +178,7 @@ static struct scsi_host_template cmd640_sht = { static struct ata_port_operations cmd640_port_ops = { .inherits = &ata_sff_port_ops, /* In theory xfer_noirq is not needed once we kill the prefetcher */ - .sff_data_xfer = ata_sff_data_xfer_noirq, + .sff_data_xfer = ata_sff_data_xfer32, .sff_irq_check = cmd640_sff_irq_check, .qc_issue = cmd640_qc_issue, .cable_detect = ata_cable_40wire, diff --git a/drivers/ata/pata_icside.c b/drivers/ata/pata_icside.c index 188f2f2eb21f..c272f2cbb47c 100644 --- a/drivers/ata/pata_icside.c +++ b/drivers/ata/pata_icside.c @@ -324,7 +324,7 @@ static struct ata_port_operations pata_icside_port_ops = { .inherits = &ata_bmdma_port_ops, /* no need to build any PRD tables for DMA */ .qc_prep = ata_noop_qc_prep, - .sff_data_xfer = ata_sff_data_xfer_noirq, + .sff_data_xfer = ata_sff_data_xfer32, .bmdma_setup = pata_icside_bmdma_setup, .bmdma_start = pata_icside_bmdma_start, .bmdma_stop = pata_icside_bmdma_stop, diff --git a/drivers/ata/pata_imx.c b/drivers/ata/pata_imx.c index d4caa23f5a88..108101325efd 100644 --- a/drivers/ata/pata_imx.c +++ b/drivers/ata/pata_imx.c @@ -102,7 +102,7 @@ static struct scsi_host_template pata_imx_sht = { static struct ata_port_operations pata_imx_port_ops = { .inherits = &ata_sff_port_ops, - .sff_data_xfer = ata_sff_data_xfer_noirq, + .sff_data_xfer = ata_sff_data_xfer32, .cable_detect = ata_cable_unknown, .set_piomode = pata_imx_set_piomode, }; diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c index 53828b6c3044..8ea4b8431fc8 100644 --- a/drivers/ata/pata_legacy.c +++ b/drivers/ata/pata_legacy.c @@ -246,12 +246,12 @@ static const struct ata_port_operations legacy_base_port_ops = { static struct ata_port_operations simple_port_ops = { .inherits = &legacy_base_port_ops, - .sff_data_xfer = ata_sff_data_xfer_noirq, + .sff_data_xfer = ata_sff_data_xfer32, }; static struct ata_port_operations legacy_port_ops = { .inherits = &legacy_base_port_ops, - .sff_data_xfer = ata_sff_data_xfer_noirq, + .sff_data_xfer = ata_sff_data_xfer32, .set_mode = legacy_set_mode, }; @@ -341,7 +341,7 @@ static unsigned int pdc_data_xfer_vlb(struct ata_queued_cmd *qc, } local_irq_restore(flags); } else - buflen = ata_sff_data_xfer_noirq(qc, buf, buflen, rw); + buflen = ata_sff_data_xfer32(qc, buf, buflen, rw); return buflen; } diff --git a/drivers/ata/pata_palmld.c b/drivers/ata/pata_palmld.c index 8c0d7d736b7a..d071ab6864a8 100644 --- a/drivers/ata/pata_palmld.c +++ b/drivers/ata/pata_palmld.c @@ -44,7 +44,7 @@ static struct scsi_host_template palmld_sht = { static struct ata_port_operations palmld_port_ops = { .inherits = &ata_sff_port_ops, - .sff_data_xfer = ata_sff_data_xfer_noirq, + .sff_data_xfer = ata_sff_data_xfer32, .cable_detect = ata_cable_40wire, }; diff --git a/drivers/ata/pata_pcmcia.c b/drivers/ata/pata_pcmcia.c index a541eacc5e95..9b0e6c72e3f9 100644 --- a/drivers/ata/pata_pcmcia.c +++ b/drivers/ata/pata_pcmcia.c @@ -151,7 +151,7 @@ static struct scsi_host_template pcmcia_sht = { static struct ata_port_operations pcmcia_port_ops = { .inherits = &ata_sff_port_ops, - .sff_data_xfer = ata_sff_data_xfer_noirq, + .sff_data_xfer = ata_sff_data_xfer32, .cable_detect = ata_cable_40wire, .set_mode = pcmcia_set_mode, }; diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c index c503ded87bb8..d6f8f5406442 100644 --- a/drivers/ata/pata_platform.c +++ b/drivers/ata/pata_platform.c @@ -49,7 +49,7 @@ static struct scsi_host_template pata_platform_sht = { static struct ata_port_operations pata_platform_port_ops = { .inherits = &ata_sff_port_ops, - .sff_data_xfer = ata_sff_data_xfer_noirq, + .sff_data_xfer = ata_sff_data_xfer32, .cable_detect = ata_cable_unknown, .set_mode = pata_platform_set_mode, }; diff --git a/drivers/ata/pata_via.c b/drivers/ata/pata_via.c index 1ca6bcab369f..fd19f1ce83aa 100644 --- a/drivers/ata/pata_via.c +++ b/drivers/ata/pata_via.c @@ -471,7 +471,7 @@ static struct ata_port_operations via_port_ops = { static struct ata_port_operations via_port_ops_noirq = { .inherits = &via_port_ops, - .sff_data_xfer = ata_sff_data_xfer_noirq, + .sff_data_xfer = ata_sff_data_xfer32, }; /** diff --git a/include/linux/libata.h b/include/linux/libata.h index ed9826b21c5e..1435e0e68823 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1831,8 +1831,6 @@ extern unsigned int ata_sff_data_xfer(struct ata_queued_cmd *qc, unsigned char *buf, unsigned int buflen, int rw); extern unsigned int ata_sff_data_xfer32(struct ata_queued_cmd *qc, unsigned char *buf, unsigned int buflen, int rw); -extern unsigned int ata_sff_data_xfer_noirq(struct ata_queued_cmd *qc, - unsigned char *buf, unsigned int buflen, int rw); extern void ata_sff_irq_on(struct ata_port *ap); extern void ata_sff_irq_clear(struct ata_port *ap); extern int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
ata_sff_data_xfer_noirq() is invoked via the ->sff_data_xfer hook. The latter is invoked by ata_pio_sector(), atapi_send_cdb() and __atapi_pio_bytes() which in turn is invoked by ata_sff_hsm_move(). The latter function requires that the "ap->lock" lock is held which needs to be taken with disabled interrupts. There is no need have to have ata_sff_data_xfer_noirq() which invokes ata_sff_data_xfer32() with disabled interrupts because at this point the interrupts are already disabled. Remove the function and its references to it and replace all callers with ata_sff_data_xfer32(). Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- Documentation/driver-api/libata.rst | 3 +-- drivers/ata/libata-sff.c | 30 ----------------------------- drivers/ata/pata_cmd640.c | 2 +- drivers/ata/pata_icside.c | 2 +- drivers/ata/pata_imx.c | 2 +- drivers/ata/pata_legacy.c | 6 +++--- drivers/ata/pata_palmld.c | 2 +- drivers/ata/pata_pcmcia.c | 2 +- drivers/ata/pata_platform.c | 2 +- drivers/ata/pata_via.c | 2 +- include/linux/libata.h | 2 -- 11 files changed, 11 insertions(+), 44 deletions(-)