Re: [PATCH] pata_parport: add driver (PARIDE replacement)

[Hannes Reinecke <hare@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

On 11/15/22 04:06, Damien Le Moal wrote:
> On 11/15/22 04:25, Ondrej Zary wrote:
>> On Monday 14 November 2022 09:03:28 Damien Le Moal wrote:
>>> On 11/14/22 16:53, Ondrej Zary wrote:
>>>> On Monday 14 November 2022, Damien Le Moal wrote:
>>>>> On 11/12/22 20:17, Ondrej Zary wrote:
>>>>>> On Wednesday 19 October 2022 09:34:31 Christoph Hellwig wrote:
>>>>>>> It's been a while - did you get a chance to make some progress on
>>>>>>> this?  Do you need any help to unblock you?
>>>>>>>
>>>>>>
>>>>>> Sorry again, I'm back now. Trying to fix locking problems.
>>>>>> Added this to each function for analysis how the functions are called wrt.
>>>>>> locking:
>>>>>>
>>>>>> 	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
>>>>>
>>>>> Do you have your code somewhere that we can look at ?
>>>>
>>>> This is the current version with debug printks. I've also added dump_stack()
>>>> to find out the code path but haven't analyzed the output yet.
>>>
>>> Can you send a proper patch ? Or a link to a git tree ? That is easier to
>>> handle than pasted code in an email...
>>
>> Patch against what? I don't have a git server.
> 
> patch against current 6.1-rc, or against an older kernel should be OK too.
> But please "git send-email" a patch, or push your dev tree to github ?
> 
Please check the attachment; I've converted Ondrey mail to a patch 
relative to the original submission.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

[-- Attachment #2: 0001-pata_parport-debug-patch-for-locking-issues.patch --]
[-- Type: text/x-patch, Size: 8992 bytes --]

From 994330bafd68da875469718064ac622e42adcc97 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@suse.de>
Date: Tue, 15 Nov 2022 08:57:51 +0100
Subject: [PATCH] pata_parport: debug patch for locking issues

Add logging messages to debug locking issues.

Signed-off-by: Ondrey Zary <linux@zary.sk>
---
 drivers/ata/pata_parport.c | 112 ++++++++++++++++++-------------------
 1 file changed, 56 insertions(+), 56 deletions(-)

diff --git a/drivers/ata/pata_parport.c b/drivers/ata/pata_parport.c
index 783764626a27..513567ce66a3 100644
--- a/drivers/ata/pata_parport.c
+++ b/drivers/ata/pata_parport.c
@@ -25,8 +25,12 @@ MODULE_PARM_DESC(verbose, "Enable verbose messages (0=off [default], 1=on)");
 
 #define DISCONNECT_TIMEOUT	(HZ / 10)
 
-static void pi_connect(struct pi_adapter *pi)
+static void pi_connect(struct ata_port *ap)
 {
+	struct pi_adapter *pi = ap->host->private_data;
+	if (spin_is_locked(ap->lock))
+		dump_stack();
+
 	del_timer_sync(&pi->timer);
 	if (!pi->claimed) {
 		pi->claimed = true;
@@ -35,10 +39,8 @@ static void pi_connect(struct pi_adapter *pi)
 	}
 }
 
-static void pi_disconnect_timer(struct timer_list *t)
+static void pi_disconnect(struct pi_adapter *pi)
 {
-	struct pi_adapter *pi = from_timer(pi, t, timer);
-
 	if (pi->claimed) {
 		pi->proto->disconnect(pi);
 		parport_release(pi->pardev);
@@ -46,18 +48,26 @@ static void pi_disconnect_timer(struct timer_list *t)
 	}
 }
 
+static void pi_disconnect_timer(struct timer_list *t)
+{
+	struct pi_adapter *pi = from_timer(pi, t, timer);
+
+	pi_disconnect(pi);
+}
+
 /* functions taken from libata-sff.c and converted from direct port I/O */
 static void pata_parport_dev_select(struct ata_port *ap, unsigned int device)
 {
 	struct pi_adapter *pi = ap->host->private_data;
 	u8 tmp;
+	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
 
 	if (device == 0)
 		tmp = ATA_DEVICE_OBS;
 	else
 		tmp = ATA_DEVICE_OBS | ATA_DEV1;
 
-	pi_connect(pi);
+	pi_connect(ap);
 	pi->proto->write_regr(pi, 0, ATA_REG_DEVICE, tmp);
 	mod_timer(&pi->timer, jiffies + DISCONNECT_TIMEOUT);
 	ata_sff_pause(ap);
@@ -67,10 +77,10 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
 {
 	struct pi_adapter *pi = ap->host->private_data;
 	u8 nsect, lbal;
-
+	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
 	pata_parport_dev_select(ap, device);
 
-	pi_connect(pi);
+	pi_connect(ap);
 	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0x55);
 	pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0xaa);
 
@@ -94,8 +104,8 @@ static int pata_parport_bus_softreset(struct ata_port *ap, unsigned int devmask,
 				      unsigned long deadline)
 {
 	struct pi_adapter *pi = ap->host->private_data;
-
-	pi_connect(pi);
+	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
+	pi_connect(ap);
 	/* software reset.  causes dev0 to be selected */
 	pi->proto->write_regr(pi, 1, 6, ap->ctl);
 	udelay(20);
@@ -116,7 +126,7 @@ static int pata_parport_softreset(struct ata_link *link, unsigned int *classes,
 	unsigned int devmask = 0;
 	int rc;
 	u8 err;
-
+	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
 	/* determine if device 0/1 are present */
 	if (pata_parport_devchk(ap, 0))
 		devmask |= (1 << 0);
@@ -147,8 +157,8 @@ static u8 pata_parport_check_status(struct ata_port *ap)
 {
 	u8 status;
 	struct pi_adapter *pi = ap->host->private_data;
-
-	pi_connect(pi);
+	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
+	pi_connect(ap);
 	status = pi->proto->read_regr(pi, 0, ATA_REG_STATUS);
 	mod_timer(&pi->timer, jiffies + DISCONNECT_TIMEOUT);
 
@@ -159,8 +169,8 @@ static u8 pata_parport_check_altstatus(struct ata_port *ap)
 {
 	u8 altstatus;
 	struct pi_adapter *pi = ap->host->private_data;
-
-	pi_connect(pi);
+	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
+	pi_connect(ap);
 	altstatus = pi->proto->read_regr(pi, 1, 6);
 	mod_timer(&pi->timer, jiffies + DISCONNECT_TIMEOUT);
 
@@ -171,8 +181,8 @@ static void pata_parport_tf_load(struct ata_port *ap,
 				 const struct ata_taskfile *tf)
 {
 	struct pi_adapter *pi = ap->host->private_data;
-
-	pi_connect(pi);
+	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
+	pi_connect(ap);
 	if (tf->ctl != ap->last_ctl) {
 		pi->proto->write_regr(pi, 1, 6, tf->ctl);
 		ap->last_ctl = tf->ctl;
@@ -209,8 +219,8 @@ static void pata_parport_tf_load(struct ata_port *ap,
 static void pata_parport_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
 {
 	struct pi_adapter *pi = ap->host->private_data;
-
-	pi_connect(pi);
+	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
+	pi_connect(ap);
 	tf->status = pi->proto->read_regr(pi, 0, ATA_REG_STATUS);
 	tf->error = pi->proto->read_regr(pi, 0, ATA_REG_ERR);
 	tf->nsect = pi->proto->read_regr(pi, 0, ATA_REG_NSECT);
@@ -236,8 +246,8 @@ static void pata_parport_exec_command(struct ata_port *ap,
 				      const struct ata_taskfile *tf)
 {
 	struct pi_adapter *pi = ap->host->private_data;
-
-	pi_connect(pi);
+	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
+	pi_connect(ap);
 	pi->proto->write_regr(pi, 0, ATA_REG_CMD, tf->command);
 	ata_sff_pause(ap);
 	mod_timer(&pi->timer, jiffies + DISCONNECT_TIMEOUT);
@@ -248,8 +258,8 @@ static unsigned int pata_parport_data_xfer(struct ata_queued_cmd *qc,
 {
 	struct ata_port *ap = qc->dev->link->ap;
 	struct pi_adapter *pi = ap->host->private_data;
-
-	pi_connect(pi);
+	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
+	pi_connect(ap);
 	if (rw == READ)
 		pi->proto->read_block(pi, buf, buflen);
 	else
@@ -271,11 +281,12 @@ static void pata_parport_drain_fifo(struct ata_queued_cmd *qc)
 		return;
 
 	ap = qc->ap;
+	printk("%s, locked=%d\n", __FUNCTION__, spin_is_locked(ap->lock));
 	pi = ap->host->private_data;
 	/* Drain up to 64K of data before we give up this recovery method */
 	for (count = 0; (pata_parport_check_status(ap) & ATA_DRQ)
 						&& count < 65536; count += 2) {
-		pi_connect(pi);
+		pi_connect(ap);
 		pi->proto->read_block(pi, junk, 2);
 		mod_timer(&pi->timer, jiffies + DISCONNECT_TIMEOUT);
 	}
@@ -284,33 +295,6 @@ static void pata_parport_drain_fifo(struct ata_queued_cmd *qc)
 		ata_port_dbg(ap, "drained %d bytes to clear DRQ\n", count);
 }
 
-static void pata_parport_lost_interrupt(struct ata_port *ap)
-{
-	u8 status;
-	struct ata_queued_cmd *qc;
-
-	/* Only one outstanding command per SFF channel */
-	qc = ata_qc_from_tag(ap, ap->link.active_tag);
-	/* We cannot lose an interrupt on a non-existent or polled command */
-	if (!qc || qc->tf.flags & ATA_TFLAG_POLLING)
-		return;
-	/*
-	 * See if the controller thinks it is still busy - if so the command
-	 * isn't a lost IRQ but is still in progress
-	 */
-	status = pata_parport_check_altstatus(ap);
-	if (status & ATA_BUSY)
-		return;
-
-	/*
-	 * There was a command running, we are no longer busy and we have
-	 * no interrupt.
-	 */
-	ata_port_warn(ap, "lost interrupt (Status 0x%x)\n", status);
-	/* Run the host interrupt logic as if the interrupt had not been lost */
-	ata_sff_port_intr(ap, qc);
-}
-
 static struct ata_port_operations pata_parport_port_ops = {
 	.inherits		= &ata_sff_port_ops,
 
@@ -325,8 +309,6 @@ static struct ata_port_operations pata_parport_port_ops = {
 	.sff_exec_command	= pata_parport_exec_command,
 	.sff_data_xfer		= pata_parport_data_xfer,
 	.sff_drain_fifo		= pata_parport_drain_fifo,
-
-	.lost_interrupt		= pata_parport_lost_interrupt,
 };
 
 static const struct ata_port_info pata_parport_port_info = {
@@ -473,6 +455,19 @@ static struct scsi_host_template pata_parport_sht = {
 	PATA_PARPORT_SHT("pata_parport")
 };
 
+struct pi_device_match {
+	struct parport *parport;
+	struct pi_protocol *proto;
+};
+
+static int pi_find_dev(struct device *dev, void *data)
+{
+	struct pi_adapter *pi = container_of(dev, struct pi_adapter, dev);
+	struct pi_device_match *match = data;
+
+	return pi->pardev->port == match->parport && pi->proto == match->proto;
+}
+
 static struct pi_adapter *pi_init_one(struct parport *parport,
 			struct pi_protocol *pr, int mode, int unit, int delay)
 {
@@ -481,6 +476,14 @@ static struct pi_adapter *pi_init_one(struct parport *parport,
 	const struct ata_port_info *ppi[] = { &pata_parport_port_info };
 	struct ata_host *host;
 	struct pi_adapter *pi;
+	struct pi_device_match match = { .parport = parport, .proto = pr };
+
+	/*
+	 * Abort if there's a device already registered on the same parport
+	 * using the same protocol.
+	 */
+	if (bus_for_each_dev(&pata_parport_bus_type, NULL, &match, pi_find_dev))
+		return NULL;
 
 	pi = kzalloc(sizeof(struct pi_adapter), GFP_KERNEL);
 	if (!pi)
@@ -674,10 +677,7 @@ static void pi_remove_one(struct device *dev)
 
 	ata_host_detach(host);
 	del_timer_sync(&pi->timer);
-	if (pi->claimed) {
-		pi->proto->disconnect(pi);
-		parport_release(pi->pardev);
-	}
+	pi_disconnect(pi);
 	pi_release(pi);
 	device_unregister(dev);
 	ida_free(&pata_parport_bus_dev_ids, dev->id);
-- 
2.35.3