| Message ID | 20211221072131.46673-1-hare@suse.de |
|---|---|
| Headers | show |
| Series | libata: rework logging, take II | expand |
On 12/21/21 16:20, Hannes Reinecke wrote: > Hi all, > > after some prodding from individual persons I've resurrected my > patchset to put libata logging on a even keel, and use structured > logging for everything. > So this patch does away with DPRINTK, ATA_DEBUG or ata_msg_XXX() > calls, and moves everything over to structured logging > (ie the dev_XXX() calls). > Additionally I've added tracepoints to trace command flow, > error handling, host state machine etc. > > So everything is looking far saner now. > > As usual, comments and reviews are welcome. > > I know that the device names suck. Blame Tejun. I applied this series, temporarily, to the branch for-5.17-logging, with the fixes I commented about. After some light testing, seems OK, but need to hammer this a little more. My main concern is the change in patch 12: using the dev_xxx() macros directly changes the messages from being all prefixed with "ataX.X" to different flavors (devX.X, linkX.X, portX.X, etc). While I do like the code simplification, this makes reading dmesg to track how a device is behaving very hard. Defining dev_fmt() everywhere the ata_{port,link,dev}_XXX functions are used would solve this, but that is annoying as almost all drivers would need to define that. Looking at the dev_XXX() macros, I do not see an easy solution. Any idea ? Note: If you resend a new version, please base it off for-5.17-logging. Cheers ! > > Changes to the original submission: > - Converted all printk() and pr_XXX() calls, too > - Included reviews from Damien > - Included reviews from Sergey > > Changes to v2: > - Rebase to ata/for-5.17 > - Include reviews from Sergey > > Hannes Reinecke (68): > libata: remove pointless debugging messages > libata: whitespace cleanup > libata: Add ata_port_classify() helper > libata: move ata_dump_id() to dynamic debugging > libata: sanitize ATA_HORKAGE_DUMP_ID > libata: add reset tracepoints > libata: add qc_prep tracepoint > libata: tracepoints for bus-master DMA > libata-sff: tracepoints for HSM state machine > libata-scsi: drop DPRINTK calls for cdb translation > libata: add tracepoints for ATA error handling > libata: move ata_{port,link,dev}_dbg to standard dev_XXX() macros > libata: revamp ata_get_cmd_descript() > libata: move DPRINTK to ata debugging > sata_mv: kill 'port' argument in mv_dump_all_regs() > sata_mv: replace DPRINTK with dynamic debugging > pata_octeon_cf: remove DPRINTK() macro in interrupt context > pdc_adma: Remove DPRINTK call > sata_fsl: move DPRINTK to ata debugging > sata_rcar: replace DPRINTK() with ata_port_dbg() > sata_qstor: replace DPRINTK() with ata_port_dbg() > pata_pdc2027x: Replace PDPRINTK() with standard ata logging > libata: remove pointless VPRINTK() calls > ahci: Drop pointless VPRINTK() calls and convert the remaining ones > pdc_adma: Drop pointless VPRINTK() calls and remove disabled NCQ > debugging > pata_octeon_cf: Drop pointless VPRINTK() calls and convert the > remaining one > pata_via: Drop pointless VPRINTK() calls > sata_promise: Drop pointless VPRINTK() calls and convert the remaining > ones > sata_qstor: Drop pointless VPRINTK() calls > sata_rcar: Drop pointless VPRINTK() calls > sata_inic162x: Drop pointless VPRINTK() calls > sata_mv: Drop pointless VPRINTK() call and convert the remaining one > sata_nv: drop pointless VPRINTK() calls and convert remaining ones > sata_fsl: convert VPRINTK() calls to ata_port_dbg() > sata_sil: Drop pointless VPRINTK() calls > sata_sx4: Drop pointless VPRINTK() calls and convert the remaining > ones > sata_sx4: add module parameter 'dimm_test' > libata: drop ata_msg_error() and ata_msg_intr() > libata: drop ata_msg_ctl() > libata: drop ata_msg_malloc() > libata: drop ata_msg_warn() > libata: drop ata_msg_probe() > libata: drop ata_msg_info() > libata: drop ata_msg_drv() > libata: remove 'new' ata message handling > libata: remove debug compilation switches > pata_atp867x: convert printk() calls > pata_cmd640: convert printk() calls > pata_cmd64x: convert printk() calls > pata_cs5520: convert printk() calls > pata_cs5536: convert printk() calls > pata_cypress: convert printk() calls > pata_it821x: convert printk() calls > pata_marvell: convert printk() calls > pata_rz1000: convert printk() calls > pata_serverworks: convert printk() calls > pata_sil680: convert printk() calls > sata_sx4: convert printk() calls > sata_mv: convert remaining printk() to structured logging > pata_hpt37x: convert pr_XXX() calls > pata_octeon_cf: Replace pr_XXX() calls with structured logging > pata_hpt3x2n: convert pr_XXX() calls > sata_gemini: convert pr_err() calls > pata_hpt366: convert pr_warn() calls > libata-scsi: rework ata_dump_status to avoid using pr_cont() > sata_dwc_460ex: drop DEBUG_NCQ > sata_dwc_460ex: remove 'check_status' argument > sata_dwc_460ex: Remove debug compile options > > drivers/ata/Kconfig | 12 - > drivers/ata/acard-ahci.c | 4 - > drivers/ata/ahci.c | 13 +- > drivers/ata/ahci_qoriq.c | 4 - > drivers/ata/ahci_xgene.c | 4 - > drivers/ata/ata_piix.c | 11 +- > drivers/ata/libahci.c | 33 +-- > drivers/ata/libata-acpi.c | 69 +++--- > drivers/ata/libata-core.c | 225 ++++------------- > drivers/ata/libata-eh.c | 72 +++--- > drivers/ata/libata-pmp.c | 8 - > drivers/ata/libata-sata.c | 5 - > drivers/ata/libata-scsi.c | 111 ++------- > drivers/ata/libata-sff.c | 88 +++---- > drivers/ata/libata-trace.c | 47 ++++ > drivers/ata/libata-transport.c | 45 +++- > drivers/ata/libata.h | 5 +- > drivers/ata/pata_arasan_cf.c | 3 + > drivers/ata/pata_atp867x.c | 29 +-- > drivers/ata/pata_cmd640.c | 2 +- > drivers/ata/pata_cmd64x.c | 4 +- > drivers/ata/pata_cs5520.c | 4 +- > drivers/ata/pata_cs5536.c | 4 +- > drivers/ata/pata_cypress.c | 2 +- > drivers/ata/pata_ep93xx.c | 1 - > drivers/ata/pata_hpt366.c | 5 +- > drivers/ata/pata_hpt37x.c | 20 +- > drivers/ata/pata_hpt3x2n.c | 12 +- > drivers/ata/pata_it821x.c | 43 ++-- > drivers/ata/pata_ixp4xx_cf.c | 6 +- > drivers/ata/pata_marvell.c | 9 +- > drivers/ata/pata_octeon_cf.c | 48 +--- > drivers/ata/pata_pdc2027x.c | 71 +++--- > drivers/ata/pata_pdc202xx_old.c | 2 - > drivers/ata/pata_rz1000.c | 4 +- > drivers/ata/pata_serverworks.c | 4 +- > drivers/ata/pata_sil680.c | 9 +- > drivers/ata/pata_via.c | 12 - > drivers/ata/pdc_adma.c | 33 +-- > drivers/ata/sata_dwc_460ex.c | 120 ++------- > drivers/ata/sata_fsl.c | 165 +++++-------- > drivers/ata/sata_gemini.c | 4 +- > drivers/ata/sata_inic162x.c | 4 +- > drivers/ata/sata_mv.c | 130 +++++----- > drivers/ata/sata_nv.c | 54 ++--- > drivers/ata/sata_promise.c | 31 +-- > drivers/ata/sata_qstor.c | 15 +- > drivers/ata/sata_rcar.c | 26 +- > drivers/ata/sata_sil.c | 1 - > drivers/ata/sata_sil24.c | 5 +- > drivers/ata/sata_sx4.c | 148 ++++-------- > include/linux/libata.h | 99 ++------ > include/trace/events/libata.h | 416 +++++++++++++++++++++++++++++++- > 53 files changed, 1050 insertions(+), 1251 deletions(-) >
On 12/30/21 6:19 AM, Damien Le Moal wrote: > On 12/21/21 16:20, Hannes Reinecke wrote: >> Hi all, >> >> after some prodding from individual persons I've resurrected my >> patchset to put libata logging on a even keel, and use structured >> logging for everything. >> So this patch does away with DPRINTK, ATA_DEBUG or ata_msg_XXX() >> calls, and moves everything over to structured logging >> (ie the dev_XXX() calls). >> Additionally I've added tracepoints to trace command flow, >> error handling, host state machine etc. >> >> So everything is looking far saner now. >> >> As usual, comments and reviews are welcome. >> >> I know that the device names suck. Blame Tejun. > > I applied this series, temporarily, to the branch for-5.17-logging, with > the fixes I commented about. > > After some light testing, seems OK, but need to hammer this a little more. > > My main concern is the change in patch 12: using the dev_xxx() macros > directly changes the messages from being all prefixed with "ataX.X" to > different flavors (devX.X, linkX.X, portX.X, etc). While I do like the > code simplification, this makes reading dmesg to track how a device is > behaving very hard. > And that was exactly what I had been hinting at with my last sentence :-) Original sin here is to have completely generic names for ata device objects (link-X? C'mon). And trying to twiddle with the devnames will land you in the mess we are in now, _and_ you lose the link between device names in sysfs and what's printed in the logging message, making debugging ever so much fun. (And I'm not even going into the _very_ slim distinction between port, link, and dev here ...) > Defining dev_fmt() everywhere the ata_{port,link,dev}_XXX functions are > used would solve this, but that is annoying as almost all drivers would > need to define that. Looking at the dev_XXX() macros, I do not see an > easy solution. Any idea ? > I guess the only sane solution would be to have a clean break by introducing a new config variable like ATA_NEW_SYSFS_NAMING. Then we could have 'sane' sysfs names like 'ata_port', 'ata_link', and 'ata_dev', _and_ would avoid the 'sysfs is ABI and cannot be modified' trap. Plan would be to send a prep patch to partially revert patch 12, so as to re-introduce the original formatting, but keeping the new accessors. And then have an additional patch to introduce the new-style sysfs layout, with the new config option. Which clearly would default to 'n' for now. Hmm? Cheers, Hannes
On 12/30/21 22:47, Hannes Reinecke wrote: > On 12/30/21 6:19 AM, Damien Le Moal wrote: >> On 12/21/21 16:20, Hannes Reinecke wrote: >>> Hi all, >>> >>> after some prodding from individual persons I've resurrected my >>> patchset to put libata logging on a even keel, and use structured >>> logging for everything. >>> So this patch does away with DPRINTK, ATA_DEBUG or ata_msg_XXX() >>> calls, and moves everything over to structured logging >>> (ie the dev_XXX() calls). >>> Additionally I've added tracepoints to trace command flow, >>> error handling, host state machine etc. >>> >>> So everything is looking far saner now. >>> >>> As usual, comments and reviews are welcome. >>> >>> I know that the device names suck. Blame Tejun. >> >> I applied this series, temporarily, to the branch for-5.17-logging, with >> the fixes I commented about. >> >> After some light testing, seems OK, but need to hammer this a little more. >> >> My main concern is the change in patch 12: using the dev_xxx() macros >> directly changes the messages from being all prefixed with "ataX.X" to >> different flavors (devX.X, linkX.X, portX.X, etc). While I do like the >> code simplification, this makes reading dmesg to track how a device is >> behaving very hard. >> > And that was exactly what I had been hinting at with my last sentence :-) > > Original sin here is to have completely generic names for ata device > objects (link-X? C'mon). > > And trying to twiddle with the devnames will land you in the mess we are > in now, _and_ you lose the link between device names in sysfs and what's > printed in the logging message, making debugging ever so much fun. > (And I'm not even going into the _very_ slim distinction between port, > link, and dev here ...) > >> Defining dev_fmt() everywhere the ata_{port,link,dev}_XXX functions are >> used would solve this, but that is annoying as almost all drivers would >> need to define that. Looking at the dev_XXX() macros, I do not see an >> easy solution. Any idea ? >> > I guess the only sane solution would be to have a clean break by > introducing a new config variable like ATA_NEW_SYSFS_NAMING. > Then we could have 'sane' sysfs names like 'ata_port', 'ata_link', and > 'ata_dev', _and_ would avoid the 'sysfs is ABI and cannot be modified' > trap. > > Plan would be to send a prep patch to partially revert patch 12, so as > to re-introduce the original formatting, but keeping the new accessors. > And then have an additional patch to introduce the new-style sysfs > layout, with the new config option. Which clearly would default to 'n' > for now. > > Hmm? For the ata_xxx_printk() functions, what about something simpler like this: +#define ata_port_printk(level, ap, fmt, ...) \ + pr_ ## level ("ata%u: " fmt, (ap)->print_id, ##__VA_ARGS__) #define ata_port_err(ap, fmt, ...) \ - ata_port_printk(ap, KERN_ERR, fmt, ##__VA_ARGS__) + ata_port_printk(err, ap, fmt, ##__VA_ARGS__) #define ata_port_warn(ap, fmt, ...) \ - ata_port_printk(ap, KERN_WARNING, fmt, ##__VA_ARGS__) + ata_port_printk(warn, ap, fmt, ##__VA_ARGS__) #define ata_port_notice(ap, fmt, ...) \ - ata_port_printk(ap, KERN_NOTICE, fmt, ##__VA_ARGS__) + ata_port_printk(notice, ap, fmt, ##__VA_ARGS__) #define ata_port_info(ap, fmt, ...) \ - ata_port_printk(ap, KERN_INFO, fmt, ##__VA_ARGS__) + ata_port_printk(info, ap, fmt, ##__VA_ARGS__) #define ata_port_dbg(ap, fmt, ...) \ - ata_port_printk(ap, KERN_DEBUG, fmt, ##__VA_ARGS__) + ata_port_printk(debug, ap, fmt, ##__VA_ARGS__) + +#define ata_link_printk(level, link, fmt, ...) \ +do { \ + if (sata_pmp_attached((link)->ap) || \ + (link)->ap->slave_link) \ + pr_ ## level ("ata%u.%02u: " fmt, \ + (link)->ap->print_id, \ + (link)->pmp, \ + ##__VA_ARGS__); \ + else \ + pr_ ## level ("ata%u: " fmt, \ + (link)->ap->print_id, \ + ##__VA_ARGS__); \ +} while (0) #define ata_link_err(link, fmt, ...) \ - ata_link_printk(link, KERN_ERR, fmt, ##__VA_ARGS__) + ata_link_printk(err, link, fmt, ##__VA_ARGS__) #define ata_link_warn(link, fmt, ...) \ - ata_link_printk(link, KERN_WARNING, fmt, ##__VA_ARGS__) + ata_link_printk(warn, link, fmt, ##__VA_ARGS__) #define ata_link_notice(link, fmt, ...) \ - ata_link_printk(link, KERN_NOTICE, fmt, ##__VA_ARGS__) + ata_link_printk(notice, link, fmt, ##__VA_ARGS__) #define ata_link_info(link, fmt, ...) \ - ata_link_printk(link, KERN_INFO, fmt, ##__VA_ARGS__) + ata_link_printk(info, link, fmt, ##__VA_ARGS__) #define ata_link_dbg(link, fmt, ...) \ - ata_link_printk(link, KERN_DEBUG, fmt, ##__VA_ARGS__) + ata_link_printk(debug, link, fmt, ##__VA_ARGS__) + +#define ata_dev_printk(level, dev, fmt, ...) \ + pr_ ## level("ata%u.%02u: " fmt, \ + (dev)->link->ap->print_id, \ + (dev)->link->pmp + (dev)->devno, \ + ##__VA_ARGS__) #define ata_dev_err(dev, fmt, ...) \ - ata_dev_printk(dev, KERN_ERR, fmt, ##__VA_ARGS__) + ata_dev_printk(err, dev, fmt, ##__VA_ARGS__) #define ata_dev_warn(dev, fmt, ...) \ - ata_dev_printk(dev, KERN_WARNING, fmt, ##__VA_ARGS__) + ata_dev_printk(warn, dev, fmt, ##__VA_ARGS__) #define ata_dev_notice(dev, fmt, ...) \ - ata_dev_printk(dev, KERN_NOTICE, fmt, ##__VA_ARGS__) + ata_dev_printk(notice, dev, fmt, ##__VA_ARGS__) #define ata_dev_info(dev, fmt, ...) \ - ata_dev_printk(dev, KERN_INFO, fmt, ##__VA_ARGS__) + ata_dev_printk(info, dev, fmt, ##__VA_ARGS__) #define ata_dev_dbg(dev, fmt, ...) \ - ata_dev_printk(dev, KERN_DEBUG, fmt, ##__VA_ARGS__) + ata_dev_printk(debug, dev, fmt, ##__VA_ARGS__) And the remaining of the patch is the same. Instead of dev_xxx() this uses pr_xxx() so we have the same dynamic debug feature and we preserve the messages as they were until now. Thoughts ? I updated the for-5.17-logging branch with this, adding the following fixes too: * Patch 21: we need to use dev_dbg() as we cannot easily get the struct ata_device for ata_dev_dbg(). * Build-bot warning fixes in drivers/ata/pata_atp867x.c (patch 47) and also in sata_mv.c (patch 16). I wonder why these 2 show up now... The warnings should exist already... Very weird. Will test more next week after restart. Happy new year ! Cheers.
On 12/31/21 5:49 AM, Damien Le Moal wrote: > On 12/30/21 22:47, Hannes Reinecke wrote: >> On 12/30/21 6:19 AM, Damien Le Moal wrote: >>> On 12/21/21 16:20, Hannes Reinecke wrote: >>>> Hi all, >>>> >>>> after some prodding from individual persons I've resurrected my >>>> patchset to put libata logging on a even keel, and use structured >>>> logging for everything. >>>> So this patch does away with DPRINTK, ATA_DEBUG or ata_msg_XXX() >>>> calls, and moves everything over to structured logging >>>> (ie the dev_XXX() calls). >>>> Additionally I've added tracepoints to trace command flow, >>>> error handling, host state machine etc. >>>> >>>> So everything is looking far saner now. >>>> >>>> As usual, comments and reviews are welcome. >>>> >>>> I know that the device names suck. Blame Tejun. >>> >>> I applied this series, temporarily, to the branch for-5.17-logging, with >>> the fixes I commented about. >>> >>> After some light testing, seems OK, but need to hammer this a little more. >>> >>> My main concern is the change in patch 12: using the dev_xxx() macros >>> directly changes the messages from being all prefixed with "ataX.X" to >>> different flavors (devX.X, linkX.X, portX.X, etc). While I do like the >>> code simplification, this makes reading dmesg to track how a device is >>> behaving very hard. >>> >> And that was exactly what I had been hinting at with my last sentence :-) >> >> Original sin here is to have completely generic names for ata device >> objects (link-X? C'mon). >> >> And trying to twiddle with the devnames will land you in the mess we are >> in now, _and_ you lose the link between device names in sysfs and what's >> printed in the logging message, making debugging ever so much fun. >> (And I'm not even going into the _very_ slim distinction between port, >> link, and dev here ...) >> >>> Defining dev_fmt() everywhere the ata_{port,link,dev}_XXX functions are >>> used would solve this, but that is annoying as almost all drivers would >>> need to define that. Looking at the dev_XXX() macros, I do not see an >>> easy solution. Any idea ? >>> >> I guess the only sane solution would be to have a clean break by >> introducing a new config variable like ATA_NEW_SYSFS_NAMING. >> Then we could have 'sane' sysfs names like 'ata_port', 'ata_link', and >> 'ata_dev', _and_ would avoid the 'sysfs is ABI and cannot be modified' >> trap. >> >> Plan would be to send a prep patch to partially revert patch 12, so as >> to re-introduce the original formatting, but keeping the new accessors. >> And then have an additional patch to introduce the new-style sysfs >> layout, with the new config option. Which clearly would default to 'n' >> for now. >> >> Hmm? > > For the ata_xxx_printk() functions, what about something simpler like this: > > +#define ata_port_printk(level, ap, fmt, ...) \ > + pr_ ## level ("ata%u: " fmt, (ap)->print_id, ##__VA_ARGS__) > > #define ata_port_err(ap, fmt, ...) \ > - ata_port_printk(ap, KERN_ERR, fmt, ##__VA_ARGS__) > + ata_port_printk(err, ap, fmt, ##__VA_ARGS__) > #define ata_port_warn(ap, fmt, ...) \ > - ata_port_printk(ap, KERN_WARNING, fmt, ##__VA_ARGS__) > + ata_port_printk(warn, ap, fmt, ##__VA_ARGS__) > #define ata_port_notice(ap, fmt, ...) \ > - ata_port_printk(ap, KERN_NOTICE, fmt, ##__VA_ARGS__) > + ata_port_printk(notice, ap, fmt, ##__VA_ARGS__) > #define ata_port_info(ap, fmt, ...) \ > - ata_port_printk(ap, KERN_INFO, fmt, ##__VA_ARGS__) > + ata_port_printk(info, ap, fmt, ##__VA_ARGS__) > #define ata_port_dbg(ap, fmt, ...) \ > - ata_port_printk(ap, KERN_DEBUG, fmt, ##__VA_ARGS__) > + ata_port_printk(debug, ap, fmt, ##__VA_ARGS__) > + > +#define ata_link_printk(level, link, fmt, ...) \ > +do { \ > + if (sata_pmp_attached((link)->ap) || \ > + (link)->ap->slave_link) \ > + pr_ ## level ("ata%u.%02u: " fmt, \ > + (link)->ap->print_id, \ > + (link)->pmp, \ > + ##__VA_ARGS__); \ > + else \ > + pr_ ## level ("ata%u: " fmt, \ > + (link)->ap->print_id, \ > + ##__VA_ARGS__); \ > +} while (0) > > #define ata_link_err(link, fmt, ...) \ > - ata_link_printk(link, KERN_ERR, fmt, ##__VA_ARGS__) > + ata_link_printk(err, link, fmt, ##__VA_ARGS__) > #define ata_link_warn(link, fmt, ...) \ > - ata_link_printk(link, KERN_WARNING, fmt, ##__VA_ARGS__) > + ata_link_printk(warn, link, fmt, ##__VA_ARGS__) > #define ata_link_notice(link, fmt, ...) \ > - ata_link_printk(link, KERN_NOTICE, fmt, ##__VA_ARGS__) > + ata_link_printk(notice, link, fmt, ##__VA_ARGS__) > #define ata_link_info(link, fmt, ...) \ > - ata_link_printk(link, KERN_INFO, fmt, ##__VA_ARGS__) > + ata_link_printk(info, link, fmt, ##__VA_ARGS__) > #define ata_link_dbg(link, fmt, ...) \ > - ata_link_printk(link, KERN_DEBUG, fmt, ##__VA_ARGS__) > + ata_link_printk(debug, link, fmt, ##__VA_ARGS__) > + > +#define ata_dev_printk(level, dev, fmt, ...) \ > + pr_ ## level("ata%u.%02u: " fmt, \ > + (dev)->link->ap->print_id, \ > + (dev)->link->pmp + (dev)->devno, \ > + ##__VA_ARGS__) > > #define ata_dev_err(dev, fmt, ...) \ > - ata_dev_printk(dev, KERN_ERR, fmt, ##__VA_ARGS__) > + ata_dev_printk(err, dev, fmt, ##__VA_ARGS__) > #define ata_dev_warn(dev, fmt, ...) \ > - ata_dev_printk(dev, KERN_WARNING, fmt, ##__VA_ARGS__) > + ata_dev_printk(warn, dev, fmt, ##__VA_ARGS__) > #define ata_dev_notice(dev, fmt, ...) \ > - ata_dev_printk(dev, KERN_NOTICE, fmt, ##__VA_ARGS__) > + ata_dev_printk(notice, dev, fmt, ##__VA_ARGS__) > #define ata_dev_info(dev, fmt, ...) \ > - ata_dev_printk(dev, KERN_INFO, fmt, ##__VA_ARGS__) > + ata_dev_printk(info, dev, fmt, ##__VA_ARGS__) > #define ata_dev_dbg(dev, fmt, ...) \ > - ata_dev_printk(dev, KERN_DEBUG, fmt, ##__VA_ARGS__) > + ata_dev_printk(debug, dev, fmt, ##__VA_ARGS__) > > And the remaining of the patch is the same. Instead of dev_xxx() this > uses pr_xxx() so we have the same dynamic debug feature and we preserve > the messages as they were until now. > > Thoughts ? > Well, yes, that would serve as a 'workaround' to restore the original names in the kernel message log. But the overall problem remains: device names in the message log do not correspond with the names in sysfs. This makes debugging really hard as one has to figure out how the device names are generated in order to find the correct sysfs entry; and I even do think that we can't distinguish between 'link' and 'device' names in the message log, making things even harder. That is something I really like to fix, and get the libata stack in line with driver core standards such that we can use the standard dev_printk() macros. Cheers, and indeed happy new year, Hannes
Hi all, after some prodding from individual persons I've resurrected my patchset to put libata logging on a even keel, and use structured logging for everything. So this patch does away with DPRINTK, ATA_DEBUG or ata_msg_XXX() calls, and moves everything over to structured logging (ie the dev_XXX() calls). Additionally I've added tracepoints to trace command flow, error handling, host state machine etc. So everything is looking far saner now. As usual, comments and reviews are welcome. I know that the device names suck. Blame Tejun. Changes to the original submission: - Converted all printk() and pr_XXX() calls, too - Included reviews from Damien - Included reviews from Sergey Changes to v2: - Rebase to ata/for-5.17 - Include reviews from Sergey Hannes Reinecke (68): libata: remove pointless debugging messages libata: whitespace cleanup libata: Add ata_port_classify() helper libata: move ata_dump_id() to dynamic debugging libata: sanitize ATA_HORKAGE_DUMP_ID libata: add reset tracepoints libata: add qc_prep tracepoint libata: tracepoints for bus-master DMA libata-sff: tracepoints for HSM state machine libata-scsi: drop DPRINTK calls for cdb translation libata: add tracepoints for ATA error handling libata: move ata_{port,link,dev}_dbg to standard dev_XXX() macros libata: revamp ata_get_cmd_descript() libata: move DPRINTK to ata debugging sata_mv: kill 'port' argument in mv_dump_all_regs() sata_mv: replace DPRINTK with dynamic debugging pata_octeon_cf: remove DPRINTK() macro in interrupt context pdc_adma: Remove DPRINTK call sata_fsl: move DPRINTK to ata debugging sata_rcar: replace DPRINTK() with ata_port_dbg() sata_qstor: replace DPRINTK() with ata_port_dbg() pata_pdc2027x: Replace PDPRINTK() with standard ata logging libata: remove pointless VPRINTK() calls ahci: Drop pointless VPRINTK() calls and convert the remaining ones pdc_adma: Drop pointless VPRINTK() calls and remove disabled NCQ debugging pata_octeon_cf: Drop pointless VPRINTK() calls and convert the remaining one pata_via: Drop pointless VPRINTK() calls sata_promise: Drop pointless VPRINTK() calls and convert the remaining ones sata_qstor: Drop pointless VPRINTK() calls sata_rcar: Drop pointless VPRINTK() calls sata_inic162x: Drop pointless VPRINTK() calls sata_mv: Drop pointless VPRINTK() call and convert the remaining one sata_nv: drop pointless VPRINTK() calls and convert remaining ones sata_fsl: convert VPRINTK() calls to ata_port_dbg() sata_sil: Drop pointless VPRINTK() calls sata_sx4: Drop pointless VPRINTK() calls and convert the remaining ones sata_sx4: add module parameter 'dimm_test' libata: drop ata_msg_error() and ata_msg_intr() libata: drop ata_msg_ctl() libata: drop ata_msg_malloc() libata: drop ata_msg_warn() libata: drop ata_msg_probe() libata: drop ata_msg_info() libata: drop ata_msg_drv() libata: remove 'new' ata message handling libata: remove debug compilation switches pata_atp867x: convert printk() calls pata_cmd640: convert printk() calls pata_cmd64x: convert printk() calls pata_cs5520: convert printk() calls pata_cs5536: convert printk() calls pata_cypress: convert printk() calls pata_it821x: convert printk() calls pata_marvell: convert printk() calls pata_rz1000: convert printk() calls pata_serverworks: convert printk() calls pata_sil680: convert printk() calls sata_sx4: convert printk() calls sata_mv: convert remaining printk() to structured logging pata_hpt37x: convert pr_XXX() calls pata_octeon_cf: Replace pr_XXX() calls with structured logging pata_hpt3x2n: convert pr_XXX() calls sata_gemini: convert pr_err() calls pata_hpt366: convert pr_warn() calls libata-scsi: rework ata_dump_status to avoid using pr_cont() sata_dwc_460ex: drop DEBUG_NCQ sata_dwc_460ex: remove 'check_status' argument sata_dwc_460ex: Remove debug compile options drivers/ata/Kconfig | 12 - drivers/ata/acard-ahci.c | 4 - drivers/ata/ahci.c | 13 +- drivers/ata/ahci_qoriq.c | 4 - drivers/ata/ahci_xgene.c | 4 - drivers/ata/ata_piix.c | 11 +- drivers/ata/libahci.c | 33 +-- drivers/ata/libata-acpi.c | 69 +++--- drivers/ata/libata-core.c | 225 ++++------------- drivers/ata/libata-eh.c | 72 +++--- drivers/ata/libata-pmp.c | 8 - drivers/ata/libata-sata.c | 5 - drivers/ata/libata-scsi.c | 111 ++------- drivers/ata/libata-sff.c | 88 +++---- drivers/ata/libata-trace.c | 47 ++++ drivers/ata/libata-transport.c | 45 +++- drivers/ata/libata.h | 5 +- drivers/ata/pata_arasan_cf.c | 3 + drivers/ata/pata_atp867x.c | 29 +-- drivers/ata/pata_cmd640.c | 2 +- drivers/ata/pata_cmd64x.c | 4 +- drivers/ata/pata_cs5520.c | 4 +- drivers/ata/pata_cs5536.c | 4 +- drivers/ata/pata_cypress.c | 2 +- drivers/ata/pata_ep93xx.c | 1 - drivers/ata/pata_hpt366.c | 5 +- drivers/ata/pata_hpt37x.c | 20 +- drivers/ata/pata_hpt3x2n.c | 12 +- drivers/ata/pata_it821x.c | 43 ++-- drivers/ata/pata_ixp4xx_cf.c | 6 +- drivers/ata/pata_marvell.c | 9 +- drivers/ata/pata_octeon_cf.c | 48 +--- drivers/ata/pata_pdc2027x.c | 71 +++--- drivers/ata/pata_pdc202xx_old.c | 2 - drivers/ata/pata_rz1000.c | 4 +- drivers/ata/pata_serverworks.c | 4 +- drivers/ata/pata_sil680.c | 9 +- drivers/ata/pata_via.c | 12 - drivers/ata/pdc_adma.c | 33 +-- drivers/ata/sata_dwc_460ex.c | 120 ++------- drivers/ata/sata_fsl.c | 165 +++++-------- drivers/ata/sata_gemini.c | 4 +- drivers/ata/sata_inic162x.c | 4 +- drivers/ata/sata_mv.c | 130 +++++----- drivers/ata/sata_nv.c | 54 ++--- drivers/ata/sata_promise.c | 31 +-- drivers/ata/sata_qstor.c | 15 +- drivers/ata/sata_rcar.c | 26 +- drivers/ata/sata_sil.c | 1 - drivers/ata/sata_sil24.c | 5 +- drivers/ata/sata_sx4.c | 148 ++++-------- include/linux/libata.h | 99 ++------ include/trace/events/libata.h | 416 +++++++++++++++++++++++++++++++- 53 files changed, 1050 insertions(+), 1251 deletions(-)