implement and use port_task
PATCH 1/1 implement and use port_task
Hello, Jeff and Albert.
This patchset implements port_task and replace pio_task and packet_task with it. port_task isn't very different from pio_task or packet_task - it's straight forward generalization. The port_task is a per-port task avaliable to low level drivers which is managed by libata core layer.
Differences from pio/packet task.
* Not limited to PIO HSM implementation. Other drivers can use it.
* Synchronization against EH core layer is done within libata core layer, not in PIO HSM implementation (ata_qc_timeout()).
Thanks.
--
tejun
PATCH 1/4 libata: implement port_task
Implement port_task. LLDD's can schedule a function to be executed with context after specified delay. libata core takes care of synchronization against EH. This is generalized form of pio_task and packet_task which are tied to PIO hsm implementation.
---
drivers/scsi/libata-core.c | 77 ++++++++++++++++++++++++++++++++++++++++++++ drivers/scsi/libata-scsi.c | 2 + drivers/scsi/libata.h | 1 + include/linux/libata.h | 4 ++ 4 files changed, 84 insertions(+), 0 deletions(-)
4d83950873856672ed469f25c6421de1eb98ba97
drivers/scsi/libata-core.c +77 -0
@@ -721,6 +721,81 @@ static unsigned int ata_pio_modes(consttiming API will get this right anyway */ } +/** +* ata_port_queue_task - Queue port_task +* @ap: The ata_port to queue port_task for +* +* Schedule @fn(@data) for execution after @delay jiffies using +* port_task. There is one port_task per port and it's the +* user(low level driver)'s responsibility to make sure that only +* one task is active at any given time. +* +* libata core layer takes care of synchronization between +* port_task and EH. ata_port_queue_task() may be ignored for EH +* synchronization. +* +* LOCKING: +* Inherited from caller. +*/ +void ata_port_queue_task (struct ata_port *ap,void (*fn)(void *),void *data, +unsigned long delay) +{ +int rc; + +if (ap->flags &ATA_FLAG_FLUSH_PIO_TASK ) +return ; + +PREPARE_WORK (&ap->port_task, fn, data); + +if (!delay) + rc =queue_work (ata_wq, &ap->port_task); +else + rc =queue_delayed_work (ata_wq, &ap->port_task, delay); + +/* rc == 0 means that another user is using port task */ +WARN_ON (rc ==0 ); +} + +/** +* ata_port_flush_task - Flush port_task +* @ap: The ata_port to flush port_task for +* +* After this function completes, port_task is guranteed not to +* be running or scheduled. +* +* LOCKING: +* Kernel thread context (may sleep) +*/ +void ata_port_flush_task (struct ata_port *ap) +{ +unsigned long flags; + +DPRINTK ("ENTER\n" ); + +spin_lock_irqsave (&ap->host_set->lock, flags); + ap->flags |=ATA_FLAG_FLUSH_PIO_TASK ; +spin_unlock_irqrestore (&ap->host_set->lock, flags); + +DPRINTK ("flush #1\n" ); +flush_workqueue (ata_wq); + +/* +* At this point, if a task is running, it's guaranteed to see +* the FLUSH flag; thus, it will never queue pio tasks again. +* Cancel and flush. +*/ +if (!cancel_delayed_work (&ap->port_task)) { +DPRINTK ("flush #2\n" ); +flush_workqueue (ata_wq); + } + +spin_lock_irqsave (&ap->host_set->lock, flags); + ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK ; +spin_unlock_irqrestore (&ap->host_set->lock, flags); + +DPRINTK ("EXIT\n" ); +} +static inline void ata_queue_packet_task (struct ata_port *ap) {
@@ -4478,6 +4553,7 @@ static void ata_host_init(struct ata_por ap->active_tag =ATA_TAG_POISON ; ap->last_ctl =0xFF ; +INIT_WORK (&ap->port_task,NULL ,NULL );INIT_WORK (&ap->packet_task, atapi_packet_task, ap);INIT_WORK (&ap->pio_task, ata_pio_task, ap);INIT_LIST_HEAD (&ap->eh_done_q);
@@ -4948,6 +5024,7 @@ EXPORT_SYMBOL_GPL(ata_drive_probe_reset)EXPORT_SYMBOL_GPL (ata_port_disable);EXPORT_SYMBOL_GPL (ata_ratelimit);EXPORT_SYMBOL_GPL (ata_busy_sleep); +EXPORT_SYMBOL_GPL (ata_port_queue_task);EXPORT_SYMBOL_GPL (ata_scsi_ioctl);EXPORT_SYMBOL_GPL (ata_scsi_queuecmd);EXPORT_SYMBOL_GPL (ata_scsi_timed_out);
drivers/scsi/libata-scsi.c +2 -0
@@ -785,6 +785,8 @@ int ata_scsi_error(struct Scsi_Host *hosWARN_ON (ata_qc_from_tag (ap, ap->active_tag) ==NULL );spin_unlock_irqrestore (&ap->host_set->lock, flags); +ata_port_flush_task (ap); + ap->ops->eng_timeout (ap);WARN_ON (host->host_failed || !list_empty (&host->eh_cmd_q));
drivers/scsi/libata.h +1 -0
@@ -45,6 +45,7 @@ extern int libata_fua;extern struct ata_queued_cmd *ata_qc_new_init (struct ata_port *ap,struct ata_device *dev);extern int ata_rwcmd_protocol (struct ata_queued_cmd *qc); +extern void ata_port_flush_task (struct ata_port *ap);extern void ata_qc_free (struct ata_queued_cmd *qc);extern unsigned int ata_qc_issue (struct ata_queued_cmd *qc);extern int ata_check_atapi_dma (struct ata_queued_cmd *qc);
include/linux/libata.h +4 -0
@@ -388,6 +388,8 @@ struct ata_port {struct ata_host_stats stats;struct ata_host_set *host_set; +struct work_struct port_task; +struct work_struct packet_task;struct work_struct pio_task;
@@ -513,6 +515,8 @@ extern int ata_ratelimit(void);extern unsigned int ata_busy_sleep (struct ata_port *ap,unsigned long timeout_pat,unsigned long timeout); +extern void ata_port_queue_task (struct ata_port *ap,void (*fn)(void *), +void *data,unsigned long delay);/* * Default driver ops implementations
Signature · 1 line
1.2.1
Re: [PATCH 1/4] libata: implement port_task
Quoted 44 lines · Tejun Heo
Implement
port_task. LLDD's can schedule a function to be executed with context after specified delay. libata core takes care of synchronization against EH. This is generalized form ofpio_taskandpacket_taskwhich are tied to PIO hsm implementation.Signed-off-by Tejun Heo <[email protected]>---
drivers/scsi/libata-core.c | 77 ++++++++++++++++++++++++++++++++++++++++++++ drivers/scsi/libata-scsi.c | 2 + drivers/scsi/libata.h | 1 + include/linux/libata.h | 4 ++ 4 files changed, 84 insertions(+), 0 deletions(-)
4d83950873856672ed469f25c6421de1eb98ba97drivers/scsi/libata-core.c +20
-0@@ -721,6 +721,81 @@ static unsigned int ata_pio_modes(consttiming API will get this right anyway */ } +/** +* ata_port_queue_task - Queue port_task +* @ap: The ata_port to queue port_task for +* +* Schedule @fn(@data) for execution after @delay jiffies using +* port_task. There is one port_task per port and it's the +* user(low level driver)'s responsibility to make sure that only +* one task is active at any given time. +* +* libata core layer takes care of synchronization between +* port_task and EH. ata_port_queue_task() may be ignored for EH +* synchronization. +* +* LOCKING: +* Inherited from caller. +*/ +void ata_port_queue_task (struct ata_port *ap,void (*fn)(void *),void *data, +unsigned long delay) +{ +int rc;
drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 753
Quoted 10 lines
drivers/scsi/libata-core.c +10
-0+ +if (ap->flags &ATA_FLAG_FLUSH_PIO_TASK ) +return ; + +PREPARE_WORK (&ap->port_task, fn, data); + +if (!delay) + rc =queue_work (ata_wq, &ap->port_task); +else + rc =queue_delayed_work (ata_wq, &ap->port_task, delay);
Two worries here, though not quite a NAK:
1) This is abuse of the PREPARE_WORK(), which is usually not used because INIT_WORK() took care of the initialization. However, it should be OK if...
2) This will fall apart if anything tries to queue a task while a previous task is still pending. Are you certain a double-queue never ever happens?
Jeff
Re: [PATCH 1/4] libata: implement port_task
Quoted 34 lines · Jeff Garzik
Quoted 21 lines
drivers/scsi/libata-core.c +17
-0+/** +* ata_port_queue_task - Queue port_task +* @ap: The ata_port to queue port_task for +* +* Schedule @fn(@data) for execution after @delay jiffies using +* port_task. There is one port_task per port and it's the +* user(low level driver)'s responsibility to make sure that only +* one task is active at any given time. +* +* libata core layer takes care of synchronization between +* port_task and EH. ata_port_queue_task() may be ignored for EH +* synchronization. +* +* LOCKING: +* Inherited from caller. +*/ +void ata_port_queue_task(struct ata_port *ap, void (*fn)(void *), void*data,
drivers/scsi/libata-core.c +3
-0+ unsigned long delay) +{ +int rc;drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 753
Quoted 10 lines
drivers/scsi/libata-core.c +10
-0+ +if (ap->flags &ATA_FLAG_FLUSH_PIO_TASK ) +return ; + +PREPARE_WORK (&ap->port_task, fn, data); + +if (!delay) + rc =queue_work (ata_wq, &ap->port_task); +else + rc =queue_delayed_work (ata_wq, &ap->port_task, delay);Two worries here, though not quite a NAK:
Quoted 3 lines
1) This is abuse of the
PREPARE_WORK(), which is usually not used becauseINIT_WORK()took care of the initialization. However, it should be OK if...
I first used separate atomic flag and INIT_WORK() for this but seemed like an overkill as task->pending does mostly the same thing.
Quoted 3 lines
2) This will fall apart if anything tries to queue a task while a previous task is still pending. Are you certain a double-queue never ever happens?
The current code never does and neither does irq-pio branch (irq-pio actually uses only one task, so...). Also, if any such event occurs, WARN_ON(rc == 0) should almost always trigger and whine about it.
We can also choose to generalize it further such that there can be more than one libata-managed tasks but there's no need yet.
Signature · 1 line
tejun
PATCH 2/4 libata: convert pio_task and packet_task to port_task
In reply to Tejun HeoMake pio_task and atapi_packet_task use port_task. atapi_packet_task() is moved upward such that it's right after ata_pio_task(). This position is more natural and makes adding prototype for ata_qc_issue_prot() unnecessary.
---
drivers/scsi/libata-core.c | 154 ++++++++++++++++++++++---------------------- 1 files changed, 77 insertions(+), 77 deletions(-)
b5b1469010fc9cd99e149f0503cbf6e6f1ac17b2
drivers/scsi/libata-core.c +77 -77
@@ -3570,12 +3570,84 @@ fsm_start: }if (timeout) -ata_queue_delayed_pio_task (ap, timeout); +ata_port_queue_task (ap, ata_pio_task, ap, timeout);else if (!qc_completed) goto fsm_start; }/** +* atapi_packet_task - Write CDB bytes to hardware +* @_data: Port to which ATAPI device is attached. +* +* When device has indicated its readiness to accept +* a CDB, this function is called. Send the CDB. +* If DMA is to be performed, exit immediately. +* Otherwise, we are in polling mode, so poll +* status under operation succeeds or fails. +* +* LOCKING: +* Kernel thread context (may sleep) +*/ + +static void atapi_packet_task (void *_data) +{ +struct ata_port *ap = _data; +struct ata_queued_cmd *qc; +u8 status; + + qc =ata_qc_from_tag (ap, ap->active_tag); +WARN_ON (qc ==NULL ); +WARN_ON (!(qc->flags &ATA_QCFLAG_ACTIVE )); + +/* sleep-wait for BSY to clear */ +DPRINTK ("busy wait\n" ); +if (ata_busy_sleep (ap,ATA_TMOUT_CDB_QUICK ,ATA_TMOUT_CDB )) { + qc->err_mask |=AC_ERR_TIMEOUT ; + goto err_out; + } + +/* make sure DRQ is set */ + status =ata_chk_status (ap); +if ((status & (ATA_BUSY |ATA_DRQ )) !=ATA_DRQ ) { + qc->err_mask |=AC_ERR_HSM ; + goto err_out; + } + +/* send SCSI cdb */ +DPRINTK ("send cdb\n" ); +WARN_ON (qc->dev->cdb_len <12 ); + +if (qc->tf.protocol ==ATA_PROT_ATAPI_DMA || + qc->tf.protocol ==ATA_PROT_ATAPI_NODATA ) { +unsigned long flags; + +/* Once we're done issuing command and kicking bmdma, +* irq handler takes over. To not lose irq, we need +* to clear NOINTR flag before sending cdb, but +* interrupt handler shouldn't be invoked before we're +* finished. Hence, the following locking. +*/ +spin_lock_irqsave (&ap->host_set->lock, flags); + ap->flags &= ~ATA_FLAG_NOINTR ; +ata_data_xfer (ap, qc->cdb, qc->dev->cdb_len,1 ); +if (qc->tf.protocol ==ATA_PROT_ATAPI_DMA ) + ap->ops->bmdma_start (qc);/* initiate bmdma */ +spin_unlock_irqrestore (&ap->host_set->lock, flags); + }else { +ata_data_xfer (ap, qc->cdb, qc->dev->cdb_len,1 ); + +/* PIO commands are handled by polling */ + ap->hsm_task_state =HSM_ST ; +ata_port_queue_task (ap, ata_pio_task, ap,0 ); + } + +return ; + +err_out: +ata_poll_qc_complete (qc); +} + +/** * ata_qc_timeout - Handle timeout of queued command * @qc: Command that timed out *
@@ -3874,26 +3946,26 @@ unsigned int ata_qc_issue_prot(struct atata_qc_set_polling (qc);ata_tf_to_host (ap, &qc->tf); ap->hsm_task_state =HSM_ST ; -ata_queue_pio_task (ap); +ata_port_queue_task (ap, ata_pio_task, ap,0 );break ;case ATA_PROT_ATAPI :ata_qc_set_polling (qc);ata_tf_to_host (ap, &qc->tf); -ata_queue_packet_task (ap); +ata_port_queue_task (ap, atapi_packet_task, ap,0 );break ;case ATA_PROT_ATAPI_NODATA : ap->flags |=ATA_FLAG_NOINTR ;ata_tf_to_host (ap, &qc->tf); -ata_queue_packet_task (ap); +ata_port_queue_task (ap, atapi_packet_task, ap,0 );break ;case ATA_PROT_ATAPI_DMA : ap->flags |=ATA_FLAG_NOINTR ; ap->ops->tf_load (ap, &qc->tf);/* load tf registers */ ap->ops->bmdma_setup (qc);/* set up bmdma */ -ata_queue_packet_task (ap); +ata_port_queue_task (ap, atapi_packet_task, ap,0 );break ;default :
@@ -4261,78 +4333,6 @@ irqreturn_t ata_interrupt (int irq, voidreturn IRQ_RETVAL (handled); } -/** -* atapi_packet_task - Write CDB bytes to hardware -* @_data: Port to which ATAPI device is attached. -* -* When device has indicated its readiness to accept -* a CDB, this function is called. Send the CDB. -* If DMA is to be performed, exit immediately. -* Otherwise, we are in polling mode, so poll -* status under operation succeeds or fails. -* -* LOCKING: -* Kernel thread context (may sleep) -*/ - -static void atapi_packet_task (void *_data) -{ -struct ata_port *ap = _data; -struct ata_queued_cmd *qc; -u8 status; - - qc =ata_qc_from_tag (ap, ap->active_tag); -WARN_ON (qc ==NULL ); -WARN_ON (!(qc->flags &ATA_QCFLAG_ACTIVE )); - -/* sleep-wait for BSY to clear */ -DPRINTK ("busy wait\n" ); -if (ata_busy_sleep (ap,ATA_TMOUT_CDB_QUICK ,ATA_TMOUT_CDB )) { - qc->err_mask |=AC_ERR_TIMEOUT ; - goto err_out; - } - -/* make sure DRQ is set */ - status =ata_chk_status (ap); -if ((status & (ATA_BUSY |ATA_DRQ )) !=ATA_DRQ ) { - qc->err_mask |=AC_ERR_HSM ; - goto err_out; - } - -/* send SCSI cdb */ -DPRINTK ("send cdb\n" ); -WARN_ON (qc->dev->cdb_len <12 ); - -if (qc->tf.protocol ==ATA_PROT_ATAPI_DMA || - qc->tf.protocol ==ATA_PROT_ATAPI_NODATA ) { -unsigned long flags; - -/* Once we're done issuing command and kicking bmdma, -* irq handler takes over. To not lose irq, we need -* to clear NOINTR flag before sending cdb, but -* interrupt handler shouldn't be invoked before we're -* finished. Hence, the following locking. -*/ -spin_lock_irqsave (&ap->host_set->lock, flags); - ap->flags &= ~ATA_FLAG_NOINTR ; -ata_data_xfer (ap, qc->cdb, qc->dev->cdb_len,1 ); -if (qc->tf.protocol ==ATA_PROT_ATAPI_DMA ) - ap->ops->bmdma_start (qc);/* initiate bmdma */ -spin_unlock_irqrestore (&ap->host_set->lock, flags); - }else { -ata_data_xfer (ap, qc->cdb, qc->dev->cdb_len,1 ); - -/* PIO commands are handled by polling */ - ap->hsm_task_state =HSM_ST ; -ata_queue_pio_task (ap); - } - -return ; - -err_out: -ata_poll_qc_complete (qc); -} -/* * Execute a 'simple' command, that only consists of the opcode 'cmd' itself,
Signature · 1 line
1.2.1
Re: [PATCH 2/4] libata: convert pio_task and packet_task to port_task
Quoted 6 lines · Tejun Heo
Make
pio_taskandatapi_packet_taskuseport_task.atapi_packet_task()is moved upward such that it's right afterata_pio_task(). This position is more natural and makes adding prototype forata_qc_issue_prot()unnecessary.Signed-off-by Tejun Heo <[email protected]>
Applied 1-4, though in the future, please -do not- include function shuffling in the same patch as other changes. atapi_packet_task() has been moved all over the place, over the lifetime of libata, and it just clutters up cumulative diffs at review time.
Jeff
PATCH 3/4 libata: kill unused pio_task and packet_task
In reply to Tejun HeoKill unused pio_task and packet_task.
---
drivers/scsi/libata-core.c | 68 -------------------------------------------- include/linux/libata.h | 3 -- 2 files changed, 0 insertions(+), 71 deletions(-)
aaf1bca7a373df0a4e301da177d6c71329b98196
drivers/scsi/libata-core.c +0 -68
@@ -796,71 +796,6 @@ void ata_port_flush_task(struct ata_portDPRINTK ("EXIT\n" ); } -static inline void -ata_queue_packet_task (struct ata_port *ap) -{ -if (!(ap->flags &ATA_FLAG_FLUSH_PIO_TASK )) -queue_work (ata_wq, &ap->packet_task); -} - -static inline void -ata_queue_pio_task (struct ata_port *ap) -{ -if (!(ap->flags &ATA_FLAG_FLUSH_PIO_TASK )) -queue_work (ata_wq, &ap->pio_task); -} - -static inline void -ata_queue_delayed_pio_task (struct ata_port *ap,unsigned long delay) -{ -if (!(ap->flags &ATA_FLAG_FLUSH_PIO_TASK )) -queue_delayed_work (ata_wq, &ap->pio_task, delay); -} - -/** -* ata_flush_pio_tasks - Flush pio_task and packet_task -* @ap: the target ata_port -* -* After this function completes, pio_task and packet_task are -* guranteed not to be running or scheduled. -* -* LOCKING: -* Kernel thread context (may sleep) -*/ - -static void ata_flush_pio_tasks (struct ata_port *ap) -{ -int tmp =0 ; -unsigned long flags; - -DPRINTK ("ENTER\n" ); - -spin_lock_irqsave (&ap->host_set->lock, flags); - ap->flags |=ATA_FLAG_FLUSH_PIO_TASK ; -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -DPRINTK ("flush #1\n" ); -flush_workqueue (ata_wq); - -/* -* At this point, if a task is running, it's guaranteed to see -* the FLUSH flag; thus, it will never queue pio tasks again. -* Cancel and flush. -*/ - tmp |=cancel_delayed_work (&ap->pio_task); - tmp |=cancel_delayed_work (&ap->packet_task); -if (!tmp) { -DPRINTK ("flush #2\n" ); -flush_workqueue (ata_wq); - } - -spin_lock_irqsave (&ap->host_set->lock, flags); - ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK ; -spin_unlock_irqrestore (&ap->host_set->lock, flags); - -DPRINTK ("EXIT\n" ); -} -void ata_qc_complete_internal (struct ata_queued_cmd *qc) {struct completion *waiting = qc->private_data;
@@ -3675,7 +3610,6 @@ static void ata_qc_timeout(struct ata_quDPRINTK ("ENTER\n" ); -ata_flush_pio_tasks (ap); ap->hsm_task_state =HSM_ST_IDLE ;spin_lock_irqsave (&host_set->lock, flags);
@@ -4554,8 +4488,6 @@ static void ata_host_init(struct ata_por ap->last_ctl =0xFF ;INIT_WORK (&ap->port_task,NULL ,NULL ); -INIT_WORK (&ap->packet_task, atapi_packet_task, ap); -INIT_WORK (&ap->pio_task, ata_pio_task, ap);INIT_LIST_HEAD (&ap->eh_done_q);for (i =0 ; i <ATA_MAX_DEVICES ; i++)
include/linux/libata.h +0 -3
@@ -390,9 +390,6 @@ struct ata_port {struct work_struct port_task; -struct work_struct packet_task; - -struct work_struct pio_task;unsigned int hsm_task_state;unsigned long pio_task_timeout;
Signature · 1 line
1.2.1
PATCH 4/4 libata: rename ATA_FLAG_FLUSH_PIO_TASK to ATA_FLAG_FLUSH_PORT_TASK
In reply to Tejun HeoRename ATA_FLAG_FLUSH_PIO_TASK to ATA_FLAG_FLUSH_PORT_TASK.
---
drivers/scsi/libata-core.c | 6 +++--- include/linux/libata.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)
4347020e90c1e2ae5d8d48b27e9d52f7143730e1
drivers/scsi/libata-core.c +3 -3
@@ -742,7 +742,7 @@ void ata_port_queue_task(struct ata_port {int rc; -if (ap->flags &ATA_FLAG_FLUSH_PIO_TASK ) +if (ap->flags &ATA_FLAG_FLUSH_PORT_TASK )return ;PREPARE_WORK (&ap->port_task, fn, data);
@@ -773,7 +773,7 @@ void ata_port_flush_task(struct ata_portDPRINTK ("ENTER\n" );spin_lock_irqsave (&ap->host_set->lock, flags); - ap->flags |=ATA_FLAG_FLUSH_PIO_TASK ; + ap->flags |=ATA_FLAG_FLUSH_PORT_TASK ;spin_unlock_irqrestore (&ap->host_set->lock, flags);DPRINTK ("flush #1\n" );
@@ -790,7 +790,7 @@ void ata_port_flush_task(struct ata_port }spin_lock_irqsave (&ap->host_set->lock, flags); - ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK ; + ap->flags &= ~ATA_FLAG_FLUSH_PORT_TASK ;spin_unlock_irqrestore (&ap->host_set->lock, flags);DPRINTK ("EXIT\n" );
include/linux/libata.h +1 -1
@@ -151,7 +151,7 @@ enum {ATA_FLAG_PIO_LBA48 = (1 <<13 ),/* Host DMA engine is LBA28 only */ ATA_FLAG_IRQ_MASK = (1 <<14 ),/* Mask IRQ in PIO xfers */ -ATA_FLAG_FLUSH_PIO_TASK = (1 <<15 ),/* Flush PIO task */ +ATA_FLAG_FLUSH_PORT_TASK = (1 <<15 ),/* Flush port task */ ATA_FLAG_IN_EH = (1 <<16 ),/* EH in progress */ ATA_QCFLAG_ACTIVE = (1 << 1), /* cmd not yet ack'd to scsi lyer */
Signature · 1 line
1.2.1