Re: [PATCH 1/3] libata: implement hotplug by polling

[Jeff Garzik <jgarzik@pobox.com>]

Tejun Heo wrote:
> This patch implements hotplug by polling - hp-poll.  It is used for
> 
> * hotplug event detection on controllers which don't provide PHY
>   status changed interrupt.
> 
> * hotplug event detection on disabled ports after reset failure.
>   libata used to leave such ports in frozen state to protect the
>   system from malfunctioning controller/device.  With hp-poll, hotplug
>   events no such ports are watched safely by polling, such that
>   removing/replacing the malfunctioning device triggers EH retry on
>   the port.
> 
> There are three port ops for hp-poll - hp_poll_activate, hp_poll,
> hp_poll_deactivate.  Only hp_poll is mandatory for hp-poll to work.
> This patch also implements SATA standard polling callbacks which poll
> SError.N/X bits - sata_std_hp_poll_activate() and sata_std_hp_poll().
> 
> By default, hp-poll is enabled only on disabled ports.  If a LLD
> doesn't support hotplug interrupts but can poll for hotplug events, it
> should indicate it by setting ATA_FLAG_HP_POLLING which tells libata
> to turn on hp-poll by default.
> 
> Putting port into any static powersave mode or setting
> libata.hotplug_polling_interval to zero disables hp-poll.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

NAK, there is no reason why a global poll list is needed.  Simply having 
a struct workqueue in each schedule-able entity implicitly creates such 
a list.  But regardless, this is another example of adding 
cross-controller synchronization, where none is needed.

If the worker fired per-host_set, then you could use an ata_port dynamic 
flag to indicate the poll-active ports, for the ata_hp_poll_worker() loop.

If the worker fires per-port, then no loop or mutex is needed at all. 
You could simply call the hp_poll hook.


> @@ -633,6 +637,10 @@ struct ata_port_operations {
>  	void (*error_handler) (struct ata_port *ap);
>  	void (*post_internal_cmd) (struct ata_queued_cmd *qc);
>  
> +	void (*hp_poll_activate) (struct ata_port *ap);
> +	void (*hp_poll_deactivate) (struct ata_port *ap);
> +	int (*hp_poll) (struct ata_port *ap);
> +
>  	irqreturn_t (*irq_handler)(int, void *, struct pt_regs *);
>  	void (*irq_clear) (struct ata_port *);
>  

All new hooks require at least a one-two sentence description in the 
DocBook docs, telling driver writers how to use them.

	Jeff