Re: Subject should be [PATCHSET libata-dev#upstream-fixes] libata: prefer hardreset, take #3

[Tejun Heo <htejun@gmail.com>]

Hello, Mikael.

Thanks for testing.

Mikael Pettersson wrote:
> Tejun Heo wrote (in first mail with wrong Subject):
>> It still needs Mikael's ack regarding behavior
>> change on sata_promise.
> 
> Unfortunately it breaks sata_promise:
> 
> ata5: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
> ata5.00: ATA-7: ST3120813AS, 3.AAD, max UDMA/133
> ata5.00: 234441648 sectors, multi 0: LBA48 NCQ (depth 0/32)
> ata5.00: configured for UDMA/133
> ata5: exception Emask 0x10 SAct 0x0 SErr 0x0 action 0xf t4
> ata5: hotplug_status 0x10
> ata5: hard resetting link

Eeeek, this means that ->hardreset is already broken on sata_promise
which gotta be fixed as it will lead to hardreset loops whenever
softreset fails.

> What happens is that a hard reset triggers cable sensing and
> reporting, so we get stray hotplug events which confuses things.
> 
> One solution is to mask/unmask a port's hotplug events around
> calls to sata_std_hardreset(). (This also matches what Promise's
> reference driver does, except their hard reset fiddles with
> Promise specific registers.) The patch below is a draft which
> implements that solution.

That can be done in either ->freeze/->thaw or ->postreset depending on
symmetry of things.  If ->freeze plugs hotplug interrupts too, it's
probably best to clear hotplug events in ->thaw before unblocking
hotplug events.

> A problem is that Promise SATA chips implement per-port hotplug
> control using port-specific bits in a single global register.
> So I have to map the link to the corresponding ata engine number:
> this mapping doesn't seem to be present anywhere, so in this
> draft patch I'm resorting to searching the host's ports[] array.

Hmmm....

> Also, since the ports share a global register it's important that
> at most one port goes through a hard reset sequence at a time.
> Can I assume that libata EH guarantees mutual exclusion?

There currently is no cross-port synchronization but you can always grab
host->lock for that.

> With this patch in place the hard reset conversion does seem to
> work Ok on a SATA300 TX4 card.
[--snip--]
> +	for(i = 0; i < 4/*XXX: how to detect # ports?*/ && host->ports[i] != ap; ++i)

host->n_ports?

> +		;
> +	if (i >= 4) {
> +		printk(KERN_ERR "%s: unable to map ap to ata_no\n", __FUNCTION__);
> +		return sata_std_hardreset(link, class, deadline);
> +	}
> +	ata_no = pdc_port_no_to_ata_no(i, is_sataii_tx4);
> +
> +	hotplug_status = readl(mmio_base + hotplug_offset);
> +
> +	/* clear hotplug flags, mask hotplug ints */
> +	hotplug_status |= (0x11 << ata_no);
> +	hotplug_status |= (0x11 << ata_no) << 16;
> +	writel(hotplug_status, mmio_base + hotplug_offset);
> +	readl(mmio_base + hotplug_offset); /* flush */

I think this belongs to ->freeze().

> +	ret = sata_std_hardreset(link, class, deadline);
> +
> +	/* clear hotplug flags, unmask hotplug ints */
> +	hotplug_status &= ~((0x11 << ata_no) << 16);
> +	writel(hotplug_status, mmio_base + hotplug_offset);
> +	readl(mmio_base + hotplug_offset); /* flush */

And this to ->thaw().

Thanks.

-- 
tejun