| Message ID | 523EF645.7050808@linux.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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> --- 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). */ -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html