Skip to contentSkip to conversation

ata_piix: minor typo fixes and threading fix

PATCH In review Levente Kurusa 5 participants 13 messages Sep 22, 2013 – Sep 27, 2013 libata

lore
Share & tools
lore

Levente Kurusa

author
#0 lore raw

PATCH 1/1 ata_piix: minor typo fixes and threading fix

Hi,

The following patch fixes a printk() call, which was originally used with pr_cont, which will however fail. Fixed that with setting up a buffer to save data to that first, and then printk() it. The patch also fixes some minor typos and a comment, so that it better reflects the
documentation of ICH*.

Regards,
Levente Kurusa

Signed-off-by Levente Kurusa <[email protected]>

---

drivers/ata/ata_piix.c +31 -13
index 93cb092..b7bf3df 100644
@@ -100,7 +100,7 @@
 
 enum {
 	PIIX_IOCFG		= 0x54, /* IDE I/O configuration register */
-	ICH5_PMR		= 0x90, /* port mapping register */
+	ICH5_PMR		= 0x90, /* address map register */
 	ICH5_PCS		= 0x92,	/* port control and status */
 	PIIX_SIDPR_BAR		= 5,
 	PIIX_SIDPR_LEN		= 16,

@@ -233,7 +233,7 @@ static const struct pci_device_id piix_pci_tbl[] = {
 	  PCI_CLASS_STORAGE_IDE << 8, 0xffff00, ich6m_sata },
 	/* 82801GB/GR/GH (ICH7, identical to ICH6) */
 	{ 0x8086, 0x27c0, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata },
-	/* 2801GBM/GHM (ICH7M, identical to ICH6M) */
+	/* 82801GBM/GHM (ICH7M, identical to ICH6M)  */
 	{ 0x8086, 0x27c4, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6m_sata },
 	/* Enterprise Southbridge 2 (631xESB/632xESB) */
 	{ 0x8086, 0x2680, PCI_ANY_ID, PCI_ANY_ID, 0, 0, ich6_sata },

@@ -515,7 +515,7 @@ static int ich_pata_cable_detect(struct ata_port *ap)
 	const struct ich_laptop *lap = &ich_laptop[0];
 	u8 mask;
 
-	/* Check for specials - Acer Aspire 5602WLMi */
+	/* Check for specials */
 	while (lap->device) {
 		if (lap->device == pdev->device &&
 		    lap->subvendor == pdev->subsystem_vendor &&

@@ -1368,34 +1368,53 @@ static const int *piix_init_sata_map(struct pci_dev *pdev,
 	pci_read_config_byte(pdev, ICH5_PMR, &map_value);
 
 	map = map_db->map[map_value & map_db->mask];
-
-	dev_info(&pdev->dev, "MAP [");
+	char* mapdata[4];
 	for (i = 0; i < 4; i++) {
 		switch (map[i]) {
 		case RV:
 			invalid_map = 1;
-			pr_cont(" XX");
+			mapdata[i] =" XX";
 			break;
 
 		case NA:
-			pr_cont(" --");
+			mapdata[i] = " --";
 			break;
 
 		case IDE:
 			WARN_ON((i & 1) || map[i + 1] != IDE);
 			pinfo[i / 2] = piix_port_info[ich_pata_100];
 			i++;
-			pr_cont(" IDE IDE");
+			mapdata[i] = " IDE IDE";
+			break;
+		case P0:
+			mapdata[i] = " P0";
+			if (i & 1)
+				pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+			break;
+		case P1:
+			mapdata[i] = " P1";
+			if (i & 1)
+				pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+			break;
+		case P2:
+			mapdata[i] = " P2";
+			if (i & 1)
+				pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+			break;
+		case P3:
+			mapdata[i] = " P3";
+			pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+			if (i & 1)
+				pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
 			break;
-
 		default:
-			pr_cont(" P%d", map[i]);
+			mapdata[i] = " INV";
 			if (i & 1)
 				pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
 			break;
 		}
 	}
-	pr_cont(" ]\n");
+	dev_info(&pdev->dev, "MAP [%s%s%s%s ]\n", mapdata[0], mapdata[1], mapdata[2], mapdata[3], map_value, map_db->mask);
 
 	if (invalid_map)
 		dev_err(&pdev->dev, "invalid MAP value %u\n", map_value);

@@ -1718,18 +1741,17 @@ static int piix_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		if (host->ports[0]->ops == &piix_sidpr_sata_ops)
 			sht = &piix_sidpr_sht;
 	}
-
 	/* apply IOCFG bit18 quirk */
 	piix_iocfg_bit18_quirk(host);
 
-	/* On ICH5, some BIOSen disable the interrupt using the
+	/* On ICH5, some BIOSes disable the interrupt using the
 	 * PCI_COMMAND_INTX_DISABLE bit added in PCI 2.3.
 	 * On ICH6, this bit has the same effect, but only when
 	 * MSI is disabled (and it is disabled, as we don't use
 	 * message-signalled interrupts currently).
 	 */
#1 lore raw

Hello.

Quoted 7 lines · Levente Kurusa

Hi,

The following patch fixes a printk() call, which was originally used with pr_cont, which will however fail. Fixed that with setting up a buffer to save data to that first, and then printk() it. The patch also
fixes some minor typos and a comment, so that it better reflects the documentation of ICH*.

Wrap your changelog lines at 80 columns (preferably less).

Quoted 2 lines

Regards,
Levente Kurusa

The patch changelog is not a place for greetings and regards.

drivers/ata/ata_piix.c line 101

Quoted 9 lines
Signed-off-by Levente Kurusa <[email protected]>

---

drivers/ata/ata_piix.c +0 -0
index 93cb092..b7bf3df 100644
@@ -100,7 +100,7 @@
 
  enum {

Your patch is whitespace damaged, there was no space on previous line, and
there are two spaces starting this line (instead of one).

drivers/ata/ata_piix.c static const int *piix_init_sata_map(struct pci_dev · line 1372

Quoted 8 lines
drivers/ata/ata_piix.c +1 -2
@@ -1368,34 +1368,53 @@ static const int *piix_init_sata_map(struct pci_dev
 *pdev,
      pci_read_config_byte(pdev, ICH5_PMR, &map_value);
 
      map = map_db->map[map_value & map_db->mask];
-
-    dev_info(&pdev->dev, "MAP [");
+    char* mapdata[4];

Don't mix declarations and data.

Quoted 31 lines
drivers/ata/ata_piix.c +15 -3
      for (i = 0; i < 4; i++) {
          switch (map[i]) {
          case RV:
              invalid_map = 1;
-            pr_cont(" XX");
+            mapdata[i] =" XX";
              break;
 
          case NA:
-            pr_cont(" --");
+            mapdata[i] = " --";
              break;
 
          case IDE:
              WARN_ON((i & 1) || map[i + 1] != IDE);
              pinfo[i / 2] = piix_port_info[ich_pata_100];
              i++;
-            pr_cont(" IDE IDE");
+            mapdata[i] = " IDE IDE";
+            break;
+        case P0:
+            mapdata[i] = " P0";
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            break;
+        case P1:
+            mapdata[i] = " P1";
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            break;
+        case P2:

drivers/ata/ata_piix.c static const int *piix_init_sata_map(struct pci_dev *pdev, · line 1409

Quoted 10 lines
drivers/ata/ata_piix.c +9 -0
+            mapdata[i] = " P2";
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            break;
+        case P3:
+            mapdata[i] = " P3";
+            pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
              break;

I don't think the code repeating 4 times is a good idea.

drivers/ata/ata_piix.c static const int *piix_init_sata_map(struct pci_dev *pdev,

Quoted 12 lines
drivers/ata/ata_piix.c +2 -3
-
          default:
-            pr_cont(" P%d", map[i]);
+            mapdata[i] = " INV";
              if (i & 1)
                  pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
              break;
          }
      }
-    pr_cont(" ]\n");
+    dev_info(&pdev->dev, "MAP [%s%s%s%s ]\n", mapdata[0], mapdata[1],
 mapdata[2], mapdata[3], map_value, map_db->mask);

You didn't specify the formats for the last 2 arguments and your line is
longer than 80 columns, please wrap it.

drivers/ata/ata_piix.c static int piix_init_one(struct pci_dev *pdev, const · line 1722

Quoted 6 lines
drivers/ata/ata_piix.c +0 -1
@@ -1718,18 +1741,17 @@ static int piix_init_one(struct pci_dev *pdev, const
 struct pci_device_id *ent)
          if (host->ports[0]->ops == &piix_sidpr_sata_ops)
              sht = &piix_sidpr_sht;
      }
-

Why?

drivers/ata/ata_piix.c static int piix_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) · line 1747

Quoted 5 lines
drivers/ata/ata_piix.c +1 -1
      /* apply IOCFG bit18 quirk */
      piix_iocfg_bit18_quirk(host);
 
-    /* On ICH5, some BIOSen disable the interrupt using the
+    /* On ICH5, some BIOSes disable the interrupt using the

Why? "BIOSen" is not a typo, it's a jargon.

WBR, Sergei

author
#2 lore raw

2013-09-22 16:44 keltezéssel, Sergei Shtylyov írta:

Quoted 13 lines

Hello.

Quoted 8 lines · Levente Kurusa

Hi,

The following patch fixes a printk() call, which was originally used with pr_cont, which will however fail. Fixed that with setting up a buffer to save
data to that first, and then printk() it. The patch also fixes some minor typos and a comment, so that it better reflects the documentation of ICH*.

Quoted 6 lines

Wrap your changelog lines at 80 columns (preferably less).

Quoted 2 lines

Regards,
Levente Kurusa

The patch changelog is not a place for greetings and regards.

Sorry for that.

Quoted 68 lines

drivers/ata/ata_piix.c line 101

Quoted 10 lines
Signed-off-by Levente Kurusa <[email protected]>

---

drivers/ata/ata_piix.c +0 -0
index 93cb092..b7bf3df 100644
@@ -100,7 +100,7 @@
 
  enum {

Your patch is whitespace damaged, there was no space on previous
line, and there are two spaces starting this line (instead of one).

drivers/ata/ata_piix.c static const int *piix_init_sata_map(struct pci_dev *pdev, · line 1371

Quoted 9 lines
drivers/ata/ata_piix.c +0 -0
@@ -1368,34 +1368,53 @@ static const int *piix_init_sata_map(struct

pci_dev
*pdev,

      pci_read_config_byte(pdev, ICH5_PMR, &map_value);

      map = map_db->map[map_value & map_db->mask];
drivers/ata/ata_piix.c +1 -2
-
-    dev_info(&pdev->dev, "MAP [");
+    char* mapdata[4];

Don't mix declarations and data.

Quoted 31 lines
      for (i = 0; i < 4; i++) {
          switch (map[i]) {
          case RV:
              invalid_map = 1;
drivers/ata/ata_piix.c +15 -3
-            pr_cont(" XX");
+            mapdata[i] =" XX";
              break;
 
          case NA:
-            pr_cont(" --");
+            mapdata[i] = " --";
              break;
 
          case IDE:
              WARN_ON((i & 1) || map[i + 1] != IDE);
              pinfo[i / 2] = piix_port_info[ich_pata_100];
              i++;
-            pr_cont(" IDE IDE");
+            mapdata[i] = " IDE IDE";
+            break;
+        case P0:
+            mapdata[i] = " P0";
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            break;
+        case P1:
+            mapdata[i] = " P1";
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            break;
+        case P2:

drivers/ata/ata_piix.c static const int *piix_init_sata_map(struct pci_dev *pdev, · line 1409

Quoted 10 lines
drivers/ata/ata_piix.c +9 -0
+            mapdata[i] = " P2";
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            break;
+        case P3:
+            mapdata[i] = " P3";
+            pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
+            if (i & 1)
+                pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
              break;
Quoted 1 line

I don't think the code repeating 4 times is a good idea.

I did this to prevent a goto, but changing that back then.

Quoted 13 lines
Quoted 12 lines
drivers/ata/ata_piix.c +2 -3
-
          default:
-            pr_cont(" P%d", map[i]);
+            mapdata[i] = " INV";
              if (i & 1)
                  pinfo[i / 2].flags |= ATA_FLAG_SLAVE_POSS;
              break;
          }
      }
-    pr_cont(" ]\n");
+    dev_info(&pdev->dev, "MAP [%s%s%s%s ]\n", mapdata[0], mapdata[1],
Quoted 1 line

mapdata[2], mapdata[3], map_value, map_db->mask);

That was from my former debug mechanism, overlooked that. Sorry.

Quoted 17 lines

You didn't specify the formats for the last 2 arguments and your
line is longer than 80 columns, please wrap it.

Quoted 7 lines
drivers/ata/ata_piix.c +0 -0
@@ -1718,18 +1741,17 @@ static int piix_init_one(struct pci_dev *pdev,

const
struct pci_device_id *ent)

          if (host->ports[0]->ops == &piix_sidpr_sata_ops)
              sht = &piix_sidpr_sht;
      }

-

Why?

Quoted 3 lines
      /* apply IOCFG bit18 quirk */
      piix_iocfg_bit18_quirk(host);
Quoted 8 lines

drivers/ata/ata_piix.c static int piix_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) · line 1747

Quoted 2 lines
drivers/ata/ata_piix.c +1 -1
-    /* On ICH5, some BIOSen disable the interrupt using the
+    /* On ICH5, some BIOSes disable the interrupt using the

Why? "BIOSen" is not a typo, it's a jargon.

WBR, Sergei

Thank you for all your comments, I am not
a native writer of english, and hence I didn't have knowledge of that jargon. I am fixing the issues, as per checkpatch.pl and your guidelines. Once finished, I will repost it.

Signature · 2 lines
Regards,
Levente Kurusa
#3 lore raw
In reply to Sergei Shtylyov
Quoted 21 lines · Sergei Shtylyov
Quoted 7 lines

Hi,

The following patch fixes a printk() call, which was originally used with pr_cont, which will however fail. Fixed that with setting up a buffer to save data to that first, and then printk() it. The patch also
fixes some minor typos and a comment, so that it better reflects the documentation of ICH*.

Wrap your changelog lines at 80 columns (preferably less).

Quoted 2 lines

Regards,
Levente Kurusa

The patch changelog is not a place for greetings and regards.

Quoted 6 lines
Signed-off-by Levente Kurusa <[email protected]>

---

drivers/ata/ata_piix.c +0 -0
index 93cb092..b7bf3df 100644

[...]

Quoted 10 lines

drivers/ata/ata_piix.c static const int *piix_init_sata_map(struct pci_dev · line 1372

Quoted 8 lines
drivers/ata/ata_piix.c +1 -2
@@ -1368,34 +1368,53 @@ static const int *piix_init_sata_map(struct pci_dev
 *pdev,
      pci_read_config_byte(pdev, ICH5_PMR, &map_value);
 
      map = map_db->map[map_value & map_db->mask];
-
-    dev_info(&pdev->dev, "MAP [");
+    char* mapdata[4];

Don't mix declarations and data.

Oops, I meant to type "code" instead of "data", of course.

WBR, Sergei

#4 lore raw
In reply to Levente Kurusa
Quoted 4 lines · Levente Kurusa

Hi,

The following patch fixes a printk() call, which was originally used with pr_cont, which will however fail. Fixed that with setting up a

Why would pr_cont() fail?

Thanks.

Signature · 1 line
tejun
#5 lore raw
Quoted 9 lines · Tejun Heo
Quoted 4 lines · Levente Kurusa

Hi,

The following patch fixes a printk() call, which was originally used with pr_cont, which will however fail. Fixed that with setting up a

Why would pr_cont() fail?

Thanks.

As far as I know pr_cont is not thread-safe. Without this patch I was only getting "... MAP [" and the rest would be scattered through the dmesg.

---
Regards,
Levente Kurusa

#6 lore raw
Quoted 3 lines · Levente Kurusa

As far as I know pr_cont is not thread-safe. Without this patch I was only getting "... MAP [" and the rest would be scattered through the dmesg.

Continuations are now properly handled by printk. Please see commits leading to eab072609e11a357181806ab5a5c309ef6eb76f5.

Thanks.

Signature · 1 line
tejun
author
#7 lore raw

I am getting the following output:

[    2.236379] ata_piix 0000:00:1f.2: MAP [
[    2.236492]  P0 P2 -- -- ]
Instead of the expected:
[    2.236379] ata_piix 0000:00:1f.2: MAP [ P0 P2 -- -- ]

This might be a minor problem, but I think it is better the second way. Of course, now that I better understand the code, I see that my fix to this, is not the best. I will make a new patch, if you think this should be fixed.

Regards,
Levente Kurusa

2013/9/22 Tejun Heo <[email protected]>:

Quoted 12 lines · Levente Kurusa
Quoted 3 lines

As far as I know pr_cont is not thread-safe. Without this patch I was only getting "... MAP [" and the rest would be scattered through the dmesg.

Continuations are now properly handled by printk. Please see commits leading to eab072609e11a357181806ab5a5c309ef6eb76f5.

Thanks.

--
tejun

#8 lore raw

(cc'ing Kay)

Quoted 9 lines · Levente Kurusa

I am getting the following output:

[    2.236379] ata_piix 0000:00:1f.2: MAP [
[    2.236492]  P0 P2 -- -- ]
Instead of the expected:
[    2.236379] ata_piix 0000:00:1f.2: MAP [ P0 P2 -- -- ]

This might be a minor problem, but I think it is better the second way. Of course, now that I better understand the code, I see that my fix to this, is not the best. I will make a new patch, if you think this should be fixed.

Hmmm... so, you're saying pr_cont() is misbehaving after dev_info()? If so, we don't want to paper over that from a low level driver like ata_piix. Let's find out what's going on with pr_cont().

Kay, Levente is seeing the above output from drivers/ata/ata_piix.c::piix_init_sata_map(). Any ideas?

Thanks.

Signature · 1 line
tejun
author
#9 lore raw

2013/9/26 Tejun Heo <[email protected]>:

Quoted 9 lines

(cc'ing Kay)

Quoted 6 lines · Levente Kurusa

I am getting the following output:

[    2.236379] ata_piix 0000:00:1f.2: MAP [
[    2.236492]  P0 P2 -- -- ]
Instead of the expected:
[    2.236379] ata_piix 0000:00:1f.2: MAP [ P0 P2 -- -- ]
Quoted 8 lines
Quoted 3 lines

This might be a minor problem, but I think it is better the second way. Of course, now that I better understand the code, I see that my fix to this, is not the best. I will make a new patch, if you think this should be fixed.

Hmmm... so, you're saying pr_cont() is misbehaving after dev_info()? If so, we don't want to paper over that from a low level driver like ata_piix. Let's find out what's going on with pr_cont().

Yes, however I am not familiar with printk() and the likes (like _dev_info()), but it looks like either _dev_info() is putting a trailing '\n' or only KERN_CONT
lines can be continued, which is probably not the way it is, because I think that sort of defeats the purpose.

Regards,
Levente Kurusa

#10 lore raw
In reply to Tejun Heo
Quoted 13 lines · Tejun Heo

(cc'ing Kay)

Quoted 9 lines · Levente Kurusa

I am getting the following output:

[    2.236379] ata_piix 0000:00:1f.2: MAP [
[    2.236492]  P0 P2 -- -- ]
Instead of the expected:
[    2.236379] ata_piix 0000:00:1f.2: MAP [ P0 P2 -- -- ]

This might be a minor problem, but I think it is better the second way. Of course, now that I better understand the code, I see that my fix to this, is not the best. I will make a new patch, if you think this should be fixed.

Quoted 6 lines

Hmmm... so, you're saying pr_cont() is misbehaving after dev_info()? If so, we don't want to paper over that from a low level driver like ata_piix. Let's find out what's going on with pr_cont().

Kay, Levente is seeing the above output from drivers/ata/ata_piix.c::piix_init_sata_map(). Any ideas?

Yes, the dev_printk() versions are "atomic lines", cannot be concatenated with other invocations of printk() like the normal printks can. The reason is that the dev_* versions carry structured data to userspace, and there, the logs are indexed by device.

We don't want to allow the convenient but very racy and non-thread-safe concatenation lines for dev_* versions, they might pick up unrelated text fragments from other threads into the logged text line; userspace (non-human tools) can not really cope with that.

So, the printk should not use the dev_* version, or print all in one call, if a single line is expected.

Kay

#11 lore raw

Hello,

Quoted 8 lines · Kay Sievers
Quoted 2 lines

Kay, Levente is seeing the above output from drivers/ata/ata_piix.c::piix_init_sata_map(). Any ideas?

Yes, the dev_printk() versions are "atomic lines", cannot be concatenated with other invocations of printk() like the normal printks can. The reason is that the dev_* versions carry structured data to userspace, and there, the logs are indexed by device.

Quoted 7 lines

We don't want to allow the convenient but very racy and non-thread-safe concatenation lines for dev_* versions, they might pick up unrelated text fragments from other threads into the logged text line; userspace (non-human tools) can not really cope with that.

So, the printk should not use the dev_* version, or print all in one call, if a single line is expected.

I see. Levente, can you please go ahead and update the patch?

Thanks.

Signature · 1 line
tejun
author
#12 lore raw
In reply to Kay Sievers

2013-09-26 19:57 keltezéssel, Kay Sievers írta:

Quoted 31 lines · Tejun Heo
Quoted 13 lines

(cc'ing Kay)

Quoted 9 lines · Levente Kurusa

I am getting the following output:

[    2.236379] ata_piix 0000:00:1f.2: MAP [
[    2.236492]  P0 P2 -- -- ]
Instead of the expected:
[    2.236379] ata_piix 0000:00:1f.2: MAP [ P0 P2 -- -- ]

This might be a minor problem, but I think it is better the second way. Of course, now that I better understand the code, I see that my fix to this, is not the best. I will make a new patch, if you think this should be fixed.

Quoted 6 lines

Hmmm... so, you're saying pr_cont() is misbehaving after dev_info()? If so, we don't want to paper over that from a low level driver like ata_piix. Let's find out what's going on with pr_cont().

Kay, Levente is seeing the above output from drivers/ata/ata_piix.c::piix_init_sata_map(). Any ideas?

Yes, the dev_printk() versions are "atomic lines", cannot be concatenated with other invocations of printk() like the normal printks can. The reason is that the dev_* versions carry structured data to userspace, and there, the logs are indexed by device.

We don't want to allow the convenient but very racy and non-thread-safe concatenation lines for dev_* versions, they might pick up unrelated text fragments from other threads into the logged text line; userspace (non-human tools) can not really cope with that.

Quoted 5 lines

So, the printk should not use the dev_* version, or print all in one call, if a single line is expected.

Kay

Thank you Kay for clarifying this! I think I will go with creating a buffer, and printing this all in one line.

Tejun, during this weekend I will create a patch that hopefully fixes this. However, if you think that setting up a temporary buffer is inappropriate, please comment.

Thank you,
Levente Kurusa