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

From: Hannes Reinecke
Date: Tue Nov 15 2022 - 03:05:02 EST

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@xxxxxxx +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
From 994330bafd68da875469718064ac622e42adcc97 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@xxxxxxx>
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@xxxxxxx>
---
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