lkml.org 
[lkml]   [2022]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] pata_parport: add driver (PARIDE replacement)
From
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
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
\
 
 \ /
  Last update: 2022-11-15 09:05    [W:0.079 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site