* [PATCH] ata_piix: minor typo fixes and threading fix
@ 2013-09-22 13:53 Levente Kurusa
2013-09-22 14:44 ` Sergei Shtylyov
2013-09-22 15:39 ` Tejun Heo
0 siblings, 2 replies; 13+ messages in thread
From: Levente Kurusa @ 2013-09-22 13:53 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide@vger.kernel.org
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 <levex@linux.com>
---
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 93cb092..b7bf3df 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -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).
*/
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-22 13:53 [PATCH] ata_piix: minor typo fixes and threading fix Levente Kurusa
@ 2013-09-22 14:44 ` Sergei Shtylyov
2013-09-22 15:05 ` Levente Kurusa
2013-09-22 15:10 ` Sergei Shtylyov
2013-09-22 15:39 ` Tejun Heo
1 sibling, 2 replies; 13+ messages in thread
From: Sergei Shtylyov @ 2013-09-22 14:44 UTC (permalink / raw)
To: levex; +Cc: Tejun Heo, linux-ide@vger.kernel.org
Hello.
On 22.09.2013 17:53, Levente Kurusa wrote:
> 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).
> Regards,
> Levente Kurusa
The patch changelog is not a place for greetings and regards.
> Signed-off-by: Levente Kurusa <levex@linux.com>
> ---
> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index 93cb092..b7bf3df 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -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).
> @@ -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.
> 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;
I don't think the code repeating 4 times is a good idea.
> -
> 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.
> @@ -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?
> /* 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-22 14:44 ` Sergei Shtylyov
@ 2013-09-22 15:05 ` Levente Kurusa
2013-09-22 15:10 ` Sergei Shtylyov
1 sibling, 0 replies; 13+ messages in thread
From: Levente Kurusa @ 2013-09-22 15:05 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: linux-ide@vger.kernel.org
2013-09-22 16:44 keltezéssel, Sergei Shtylyov írta:
> Hello.
>
> On 22.09.2013 17:53, Levente Kurusa wrote:
>
>> 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).
>
>> Regards,
>> Levente Kurusa
>
> The patch changelog is not a place for greetings and regards.
Sorry for that.
>
>> Signed-off-by: Levente Kurusa <levex@linux.com>
>> ---
>> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
>> index 93cb092..b7bf3df 100644
>> --- a/drivers/ata/ata_piix.c
>> +++ b/drivers/ata/ata_piix.c
>> @@ -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).
>
>> @@ -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.
>
>> 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;
>
> 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.
>
>> -
>> 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);
That was from my former debug mechanism, overlooked that. Sorry.
>
> You didn't specify the formats for the last 2 arguments and your
> line is longer than 80 columns, please wrap it.
>
>> @@ -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?
>
>> /* 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
>
>
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.
--
Regards,
Levente Kurusa
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-22 14:44 ` Sergei Shtylyov
2013-09-22 15:05 ` Levente Kurusa
@ 2013-09-22 15:10 ` Sergei Shtylyov
1 sibling, 0 replies; 13+ messages in thread
From: Sergei Shtylyov @ 2013-09-22 15:10 UTC (permalink / raw)
To: levex; +Cc: Tejun Heo, linux-ide@vger.kernel.org
On 22-09-2013 18:44, Sergei Shtylyov wrote:
>> 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).
>> Regards,
>> Levente Kurusa
> The patch changelog is not a place for greetings and regards.
>> Signed-off-by: Levente Kurusa <levex@linux.com>
>> ---
>> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
>> index 93cb092..b7bf3df 100644
>> --- a/drivers/ata/ata_piix.c
>> +++ b/drivers/ata/ata_piix.c
[...]
>> @@ -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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-22 13:53 [PATCH] ata_piix: minor typo fixes and threading fix Levente Kurusa
2013-09-22 14:44 ` Sergei Shtylyov
@ 2013-09-22 15:39 ` Tejun Heo
2013-09-22 15:46 ` Levente Kurusa
1 sibling, 1 reply; 13+ messages in thread
From: Tejun Heo @ 2013-09-22 15:39 UTC (permalink / raw)
To: Levente Kurusa; +Cc: linux-ide@vger.kernel.org
On Sun, Sep 22, 2013 at 03:53:09PM +0200, Levente Kurusa wrote:
> 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.
--
tejun
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-22 15:39 ` Tejun Heo
@ 2013-09-22 15:46 ` Levente Kurusa
2013-09-22 16:05 ` Tejun Heo
0 siblings, 1 reply; 13+ messages in thread
From: Levente Kurusa @ 2013-09-22 15:46 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide
On 2013-09-22 17:39, Tejun Heo wrote:
> On Sun, Sep 22, 2013 at 03:53:09PM +0200, Levente Kurusa wrote:
>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-22 15:46 ` Levente Kurusa
@ 2013-09-22 16:05 ` Tejun Heo
2013-09-25 14:40 ` Levente Kurusa
0 siblings, 1 reply; 13+ messages in thread
From: Tejun Heo @ 2013-09-22 16:05 UTC (permalink / raw)
To: Levente Kurusa; +Cc: linux-ide
On Sun, Sep 22, 2013 at 05:46:03PM +0200, Levente Kurusa wrote:
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-22 16:05 ` Tejun Heo
@ 2013-09-25 14:40 ` Levente Kurusa
2013-09-26 14:09 ` Tejun Heo
0 siblings, 1 reply; 13+ messages in thread
From: Levente Kurusa @ 2013-09-25 14:40 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide
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 <tj@kernel.org>:
> On Sun, Sep 22, 2013 at 05:46:03PM +0200, Levente Kurusa wrote:
>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-25 14:40 ` Levente Kurusa
@ 2013-09-26 14:09 ` Tejun Heo
2013-09-26 17:14 ` Levente Kurusa
2013-09-26 17:57 ` Kay Sievers
0 siblings, 2 replies; 13+ messages in thread
From: Tejun Heo @ 2013-09-26 14:09 UTC (permalink / raw)
To: Levente Kurusa; +Cc: linux-ide, Kay Sievers
(cc'ing Kay)
On Wed, Sep 25, 2013 at 04:40:30PM +0200, Levente Kurusa wrote:
> 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.
--
tejun
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-26 14:09 ` Tejun Heo
@ 2013-09-26 17:14 ` Levente Kurusa
2013-09-26 17:57 ` Kay Sievers
1 sibling, 0 replies; 13+ messages in thread
From: Levente Kurusa @ 2013-09-26 17:14 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, Kay Sievers
2013/9/26 Tejun Heo <tj@kernel.org>:
> (cc'ing Kay)
>
> On Wed, Sep 25, 2013 at 04:40:30PM +0200, Levente Kurusa wrote:
>> 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().
>
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-26 14:09 ` Tejun Heo
2013-09-26 17:14 ` Levente Kurusa
@ 2013-09-26 17:57 ` Kay Sievers
2013-09-27 13:28 ` Tejun Heo
2013-09-27 13:29 ` Levente Kurusa
1 sibling, 2 replies; 13+ messages in thread
From: Kay Sievers @ 2013-09-26 17:57 UTC (permalink / raw)
To: Tejun Heo; +Cc: Levente Kurusa, linux-ide
On Thu, Sep 26, 2013 at 4:09 PM, Tejun Heo <tj@kernel.org> wrote:
> (cc'ing Kay)
>
> On Wed, Sep 25, 2013 at 04:40:30PM +0200, Levente Kurusa wrote:
>> 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?
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-26 17:57 ` Kay Sievers
@ 2013-09-27 13:28 ` Tejun Heo
2013-09-27 13:29 ` Levente Kurusa
1 sibling, 0 replies; 13+ messages in thread
From: Tejun Heo @ 2013-09-27 13:28 UTC (permalink / raw)
To: Kay Sievers; +Cc: Levente Kurusa, linux-ide
Hello,
On Thu, Sep 26, 2013 at 07:57:35PM +0200, Kay Sievers wrote:
> > 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.
I see. Levente, can you please go ahead and update the patch?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ata_piix: minor typo fixes and threading fix
2013-09-26 17:57 ` Kay Sievers
2013-09-27 13:28 ` Tejun Heo
@ 2013-09-27 13:29 ` Levente Kurusa
1 sibling, 0 replies; 13+ messages in thread
From: Levente Kurusa @ 2013-09-27 13:29 UTC (permalink / raw)
To: Kay Sievers, Tejun Heo; +Cc: linux-ide
2013-09-26 19:57 keltezéssel, Kay Sievers írta:
> On Thu, Sep 26, 2013 at 4:09 PM, Tejun Heo <tj@kernel.org> wrote:
>> (cc'ing Kay)
>>
>> On Wed, Sep 25, 2013 at 04:40:30PM +0200, Levente Kurusa wrote:
>>> 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?
>
> 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
>
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-09-27 13:29 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-22 13:53 [PATCH] ata_piix: minor typo fixes and threading fix Levente Kurusa
2013-09-22 14:44 ` Sergei Shtylyov
2013-09-22 15:05 ` Levente Kurusa
2013-09-22 15:10 ` Sergei Shtylyov
2013-09-22 15:39 ` Tejun Heo
2013-09-22 15:46 ` Levente Kurusa
2013-09-22 16:05 ` Tejun Heo
2013-09-25 14:40 ` Levente Kurusa
2013-09-26 14:09 ` Tejun Heo
2013-09-26 17:14 ` Levente Kurusa
2013-09-26 17:57 ` Kay Sievers
2013-09-27 13:28 ` Tejun Heo
2013-09-27 13:29 ` Levente Kurusa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).