Re: [PATCH v2 0/5] libsas: Some logging tidy-up
On Thu, 2018-11-15 at 18:20 +0800, John Garry wrote: > This patchset presents some logging tidy-up, namely removing the printk > wrappers and using pr_XXX() instead. > > In addition, some log levels are revised, as previous levels (generally > debug) were too low. Seems sensible, thanks.
Re: [PATCH 4/5] scsi: libsas: Drop SAS_DPRINTK() and revise logs levels
On Wed, 2018-11-14 at 15:12 +, John Garry wrote: > On 14/11/2018 14:53, Joe Perches wrote: > > On Wed, 2018-11-14 at 21:47 +0800, John Garry wrote: > > > Like sas_printk() did previously, SAS_DPRINTK() offers little value now > > > that libsas logs already have the "sas" prefix through pr_fmt(fmt). So it > > > can be dropped. > > > > > > However, after reviewing some logs in libsas, it is noticed that debug > > > level is too low in many instances. > > > > > > So this change drops SAS_DPRINTK() and revises some logs to a more > > > appropriate level. However many stay at debug level, although some > > > are significantly promoted. > > [] > > > All the pre-existing checkpatch errors for spanning messages across > > > multiple lines are untouched. > > > > I think coalescing would be useful. > > Sorry, I missed that. Do you mean that we stop spanning strings over > multiple lines? Yes. > If yes, I tend to agree. It means we can grep for full strings vs just a > different checkpatch issue (>80 lines or spanning multiple lines) checkpatch does not emit a warning when a string is the last element of a line that is > 80 chars when the string stats before position 79.
Re: [PATCH 4/5] scsi: libsas: Drop SAS_DPRINTK() and revise logs levels
On Wed, 2018-11-14 at 21:47 +0800, John Garry wrote:
> Like sas_printk() did previously, SAS_DPRINTK() offers little value now
> that libsas logs already have the "sas" prefix through pr_fmt(fmt). So it
> can be dropped.
>
> However, after reviewing some logs in libsas, it is noticed that debug
> level is too low in many instances.
>
> So this change drops SAS_DPRINTK() and revises some logs to a more
> appropriate level. However many stay at debug level, although some
> are significantly promoted.
[]
> All the pre-existing checkpatch errors for spanning messages across
> multiple lines are untouched.
I think coalescing would be useful.
Where there are already embedded "sas: "
prefixes, those should be removed too.
You could verify this by using
$ strings drivers/scsi/libsas/*.o | grep "^[0-7]"
> @@ -186,10 +186,10 @@ int sas_notify_lldd_dev_found(struct domain_device *dev)
>
> res = i->dft->lldd_dev_found(dev);
> if (res) {
> - printk("sas: driver on pcidev %s cannot handle "
> -"device %llx, error:%d\n",
> -dev_name(sas_ha->dev),
> -SAS_ADDR(dev->sas_addr), res);
> + pr_warn("sas: driver on pcidev %s cannot handle "
> + "device %llx, error:%d\n",
> + dev_name(sas_ha->dev),
> + SAS_ADDR(dev->sas_addr), res);
e.g.: this now emits "sas: sas: driver etc..."
Re: [PATCH] scsi: libsas: Remove pcidev reference
On Tue, 2018-11-13 at 13:38 +, John Garry wrote:
> On 12/11/2018 19:52, Joe Perches wrote:
> > On Mon, 2018-11-12 at 19:31 +, John Garry wrote:
> > > On 12/11/2018 18:58, Joe Perches wrote:
> > > > > > +#define pr_fmt(fmt) "sas: " fmt
[]
> > > > It also might useful to use the common debugging
> > > > mechanisms and convert SAS_DPRINTK to pr_debug
> > > > which would use the same #define pr_fmt.
{}
> We have almost 100 references to SAS_DPRINTK. So when we replace with
> pr_debug, we will frequently have to realign the indentation. I'm
> beginning to think it's not worth the churn to remove SAS_DPRINTK.
or just convert the #define
> However, with regards to using pr_debug(), there are loops to jump
> through to ensure any output at all:
> - ensure DEBUG is defined or DYNAMIC DEBUG enabled
True
> - ensure current loglevel is high enough
That's already the case.
> Having said that, there are actually lots of logs in libsas where debug
> level is too low (at pr_debug), and these could be upgraded.
>
> So I think I will just go through the code and revise these levels.
Nice, thanks
Re: [PATCH] scsi: libsas: Remove pcidev reference
On Mon, 2018-11-12 at 19:31 +, John Garry wrote: > On 12/11/2018 18:58, Joe Perches wrote: > > > > +#define pr_fmt(fmt) "sas: " fmt > > > > > > > > Some other subsystem may try to include this header, and gets its > > > > message prefix overwritten. Just a consequence for doing something bad, > > > > right? > > Right. > > > > And as this file is internal to drivers/scsi/libsas > > that seems very unlikely to occur. > > > > It also might useful to use the common debugging > > mechanisms and convert SAS_DPRINTK to pr_debug > > which would use the same #define pr_fmt. > > > > OK, I will try to put this all together as a marginally wider scope tidy-up. Thanks. Another thing that could be done is to change the #define pr_fmt(fmt) to KBUILD_MODNAME as that would prefix "libsas" instead of just "sas". I think that would be better but I didn't do that as it should be in a separate patch.
Re: [PATCH] scsi: libsas: Remove pcidev reference
On Mon, 2018-11-12 at 18:48 +, John Garry wrote: > On 12/11/2018 18:30, Joe Perches wrote: [] > > diff --git a/drivers/scsi/libsas/sas_internal.h > > b/drivers/scsi/libsas/sas_internal.h [] > > @@ -32,7 +32,10 @@ > > #include > > #include > > > > -#define sas_printk(fmt, ...) printk(KERN_NOTICE "sas: " fmt, ## > > __VA_ARGS__) > > +#ifdef pr_fmt > > +#undef pr_fmt > > +#endif > > +#define pr_fmt(fmt) "sas: " fmt > > Some other subsystem may try to include this header, and gets its > message prefix overwritten. Just a consequence for doing something bad, > right? Right. And as this file is internal to drivers/scsi/libsas that seems very unlikely to occur. It also might useful to use the common debugging mechanisms and convert SAS_DPRINTK to pr_debug which would use the same #define pr_fmt.
Re: [PATCH] scsi: libsas: Remove pcidev reference
On Mon, 2018-11-12 at 17:55 +, John Garry wrote:
> On 12/11/2018 17:49, John Garry wrote:
> > On 12/11/2018 17:32, Joe Perches wrote:
> > > On Tue, 2018-11-13 at 01:28 +0800, John Garry wrote:
> > > > Not all host drivers are PCI drivers - like hisi_sas, which supports a
> > > > platform driver - so remove reference to "pcidev".
> > > >
> > > > The debug level is also downgraded to KERN_ERR for the same message.
> > > []
> > > > diff --git a/drivers/scsi/libsas/sas_discover.c
> > > > b/drivers/scsi/libsas/sas_discover.c
> > > []
> > > > @@ -186,7 +186,7 @@ int sas_notify_lldd_dev_found(struct
> > > > domain_device *dev)
> > > >
> > > > res = i->dft->lldd_dev_found(dev);
> > > > if (res) {
> > > > -printk("sas: driver on pcidev %s cannot handle "
> > > > +pr_err("sas: driver on host %s cannot handle "
> > > > "device %llx, error:%d\n",
> > >
> > > As a printk without a KERN_ is printed at whatever
> > > CONFIG_MESSAGE_LOGLEVEL_DEFAULT is set to (default: 4 and
> > > rarely unchanged), this is effectively upgraded from a
> > > KERN_WARNING to KERN_ERR.
> >
> > ah, I thought that it was printed always.
> >
> > So maybe I'll just leave as-is.
>
> I forgot to mention that checkpatch complains about using printk() -
> that's why I changed it.
I believe that always assigning a KERN_
is good thing so I am not against this change.
Describing the change a bit more correctly than
'debug level downgraded' would be useful.
My preference is a patch like this which
always prefixes "sas: " to each log message
by adding a #define pr_fmt and converts the
bare printks to pr_notice, and converts the
slightly odd sas_printk macro and uses with
a hidden KERN_NOTICE to pr_notice.
---
drivers/scsi/libsas/sas_discover.c | 9 -
drivers/scsi/libsas/sas_expander.c | 33 -
drivers/scsi/libsas/sas_init.c | 10 +-
drivers/scsi/libsas/sas_internal.h | 5 -
drivers/scsi/libsas/sas_phy.c | 10 +-
drivers/scsi/libsas/sas_port.c | 3 +--
6 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/drivers/scsi/libsas/sas_discover.c
b/drivers/scsi/libsas/sas_discover.c
index dde433aa59c2..193d7a1ebeb3 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -128,7 +128,8 @@ static int sas_get_port_device(struct asd_sas_port *port)
SAS_FANOUT_EXPANDER_DEVICE);
break;
default:
- printk("ERROR: Unidentified device type %d\n", dev->dev_type);
+ pr_notice("ERROR: Unidentified device type %d\n",
+ dev->dev_type);
rphy = NULL;
break;
}
@@ -186,10 +187,8 @@ int sas_notify_lldd_dev_found(struct domain_device *dev)
res = i->dft->lldd_dev_found(dev);
if (res) {
- printk("sas: driver on pcidev %s cannot handle "
- "device %llx, error:%d\n",
- dev_name(sas_ha->dev),
- SAS_ADDR(dev->sas_addr), res);
+ pr_notice("driver on pcidev %s cannot handle device %llx,
error:%d\n",
+ dev_name(sas_ha->dev), SAS_ADDR(dev->sas_addr), res);
}
set_bit(SAS_DEV_FOUND, &dev->state);
kref_get(&dev->kref);
diff --git a/drivers/scsi/libsas/sas_expander.c
b/drivers/scsi/libsas/sas_expander.c
index 0d1f72752ca2..922253f1b05f 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -393,7 +393,7 @@ static int sas_ex_phy_discover_helper(struct domain_device
*dev, u8 *disc_req,
return res;
dr = &((struct smp_resp *)disc_resp)->disc;
if (memcmp(dev->sas_addr, dr->attached_sas_addr, SAS_ADDR_SIZE) == 0) {
- sas_printk("Found loopback topology, just ignore it!\n");
+ pr_notice("Found loopback topology, just ignore it!\n");
return 0;
}
sas_set_ex_phy(dev, single, disc_resp);
@@ -1262,19 +1262,18 @@ static void sas_print_parent_topology_bug(struct
domain_device *child,
};
struct domain_device *parent = child->parent;
- sas_printk("%s ex %016llx phy 0x%x <--> %s ex %016llx "
- "phy 0x%x has %c:%c routing link!\n",
+ pr_notice("%s ex %016llx phy 0x%x <--> %s ex %016llx phy 0x%x has %c:%c
routing link!\n&q
Re: [PATCH] scsi: libsas: Remove pcidev reference
On Tue, 2018-11-13 at 01:28 +0800, John Garry wrote:
> Not all host drivers are PCI drivers - like hisi_sas, which supports a
> platform driver - so remove reference to "pcidev".
>
> The debug level is also downgraded to KERN_ERR for the same message.
[]
> diff --git a/drivers/scsi/libsas/sas_discover.c
> b/drivers/scsi/libsas/sas_discover.c
[]
> @@ -186,7 +186,7 @@ int sas_notify_lldd_dev_found(struct domain_device *dev)
>
> res = i->dft->lldd_dev_found(dev);
> if (res) {
> - printk("sas: driver on pcidev %s cannot handle "
> + pr_err("sas: driver on host %s cannot handle "
> "device %llx, error:%d\n",
As a printk without a KERN_ is printed at whatever
CONFIG_MESSAGE_LOGLEVEL_DEFAULT is set to (default: 4 and
rarely unchanged), this is effectively upgraded from a
KERN_WARNING to KERN_ERR.
Re: [PATCH] scsi: aic7xxx: Fix unintended sign extension issue
On Thu, 2018-10-25 at 16:13 +0100, Colin King wrote:
> From: Colin Ian King
>
> In the expression "ahc_inb(ahc, port+3) << 24", the initial value is a
> u8, but is promoted to a signed int, then sign-extended to uint64_t. If
> the value read from the port has the upper bit set then the sign
> extension will set all the upper bits of the expression which is probably
> not what was intended. Cast to uint64_t to avoid the sign extension.
[]
> diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c
> b/drivers/scsi/aic7xxx/aic79xx_core.c
[]
> @@ -622,7 +622,7 @@ ahd_inq(struct ahd_softc *ahd, u_int port)
> return ((ahd_inb(ahd, port))
> | (ahd_inb(ahd, port+1) << 8)
> | (ahd_inb(ahd, port+2) << 16)
> - | (ahd_inb(ahd, port+3) << 24)
> + | (((uint64_t)ahd_inb(ahd, port+3)) << 24)
> | (((uint64_t)ahd_inb(ahd, port+4)) << 32)
> | (((uint64_t)ahd_inb(ahd, port+5)) << 40)
> | (((uint64_t)ahd_inb(ahd, port+6)) << 48)
Perhaps a different method using two calls to ahd_inl
is clearer and possibly faster like:
uint64_t
ahd_inq(struct ahd_softc *ahd, u_int port)
{
return ahd_inl(port) | ((uint64_t)ahd_inl(port + 4) << 32);
}
> diff --git a/drivers/scsi/aic7xxx/aic7xxx_core.c
> b/drivers/scsi/aic7xxx/aic7xxx_core.c
[]
> @@ -493,7 +493,7 @@ ahc_inq(struct ahc_softc *ahc, u_int port)
> return ((ahc_inb(ahc, port))
> | (ahc_inb(ahc, port+1) << 8)
> | (ahc_inb(ahc, port+2) << 16)
> - | (ahc_inb(ahc, port+3) << 24)
> + | (((uint64_t)ahc_inb(ahc, port+3)) << 24)
> | (((uint64_t)ahc_inb(ahc, port+4)) << 32)
> | (((uint64_t)ahc_inb(ahc, port+5)) << 40)
> | (((uint64_t)ahc_inb(ahc, port+6)) << 48)
here too
Re: [PATCH 0/7] Neaten logging uses
On Thu, 2018-10-11 at 19:25 -0400, Martin K. Petersen wrote: > Joe, > > > > Applied to 4.20/scsi-queue, thanks! > > > > Is this branch also applied to Stephen Rothwell's -next? > > Yes. Via what tree? It doesn't seem directly pulled.
Re: [PATCH 0/7] Neaten logging uses
On Wed, 2018-10-10 at 21:54 -0400, Martin K. Petersen wrote: > Joe, > > > Several defects exist in the logging uses > > > > o Missing KERN_ > > o Unnecessary KERN_ uses with panic > > o Mismatched MPT3SAS_FMT and %s: with name and __func__ > > > > Correct these defects and perhaps add some clarity to the logging. > > Applied to 4.20/scsi-queue, thanks! Is this branch also applied to Stephen Rothwell's -next?
Re: [PATCH 0/7] Neaten logging uses
On Mon, 2018-09-17 at 08:01 -0700, Joe Perches wrote: > Several defects exist in the logging uses > > o Missing KERN_ > o Unnecessary KERN_ uses with panic > o Mismatched MPT3SAS_FMT and %s: with name and __func__ > > Correct these defects and perhaps add some clarity to the logging. Submitted 2 weeks ago now without comment. ping... > Joe Perches (7): > mpt3sas: Add ioc_ logging macros > mpt3sas: Convert uses of pr_ with MPT3SAS_FMT to ioc_ > mpt3sas: Convert mlsleading uses of pr_ with MPT3SAS_FMT > mpt3sas: Convert logging uses with MPT3SAS_FMT and reply_q_name to %s: > mpt3sas: Remove KERN_WARNING from panic uses > mpt3sas: Convert logging uses with MPT3SAS_FMT without logging levels > mpt3sas: Remove unused macro MPT3SAS_FMT > > drivers/scsi/mpt3sas/mpt3sas_base.c | 1116 +--- > drivers/scsi/mpt3sas/mpt3sas_base.h |9 +- > drivers/scsi/mpt3sas/mpt3sas_config.c | 89 +- > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 493 - > drivers/scsi/mpt3sas/mpt3sas_scsih.c| 1487 > --- > drivers/scsi/mpt3sas/mpt3sas_transport.c| 337 +++--- > drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c | 101 +- > drivers/scsi/mpt3sas/mpt3sas_warpdrive.c| 70 +- > 8 files changed, 1618 insertions(+), 2084 deletions(-) >
Re: [PATCH] scsi: arcmsr: clean up clang warning on extraneous parentheses
On Mon, 2018-10-01 at 00:03 +0100, Colin King wrote:
> From: Colin Ian King
>
> There are extraneous parantheses that are causing clang to produce a
> warning so remove these.
>
> Clean up 3 clang warnings:
> equality comparison with extraneous parentheses [-Wparentheses-equality]
[]
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c
> b/drivers/scsi/arcmsr/arcmsr_hba.c
[]
> @@ -4135,9 +4135,9 @@ static void arcmsr_hardware_reset(struct
> AdapterControlBlock *acb)
> pci_read_config_byte(acb->pdev, i, &value[i]);
> }
> /* hardware reset signal */
> - if ((acb->dev_id == 0x1680)) {
> + if (acb->dev_id == 0x1680) {
> writel(ARCMSR_ARC1680_BUS_RESET, &pmuA->reserved1[0]);
> - } else if ((acb->dev_id == 0x1880)) {
> + } else if (acb->dev_id == 0x1880) {
> do {
> count++;
> writel(0xF, &pmuC->write_sequence);
> @@ -4161,7 +4161,7 @@ static void arcmsr_hardware_reset(struct
> AdapterControlBlock *acb)
> } while (((readl(&pmuE->host_diagnostic_3xxx) &
> ARCMSR_ARC1884_DiagWrite_ENABLE) == 0) && (count < 5));
> writel(ARCMSR_ARC188X_RESET_ADAPTER,
> &pmuE->host_diagnostic_3xxx);
> - } else if ((acb->dev_id == 0x1214)) {
> + } else if (acb->dev_id == 0x1214) {
> writel(0x20, pmuD->reset_request);
> } else {
> pci_write_config_byte(acb->pdev, 0x84, 0x20);
It'd probably read better using a switch/case like the below:
---
drivers/scsi/arcmsr/arcmsr_hba.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 12316ef4c893..ad45583f5ae0 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -4135,9 +4135,11 @@ static void arcmsr_hardware_reset(struct
AdapterControlBlock *acb)
pci_read_config_byte(acb->pdev, i, &value[i]);
}
/* hardware reset signal */
- if ((acb->dev_id == 0x1680)) {
+ switch (acb->dev_id) {
+ case 0x1680:
writel(ARCMSR_ARC1680_BUS_RESET, &pmuA->reserved1[0]);
- } else if ((acb->dev_id == 0x1880)) {
+ break;
+ case 0x1880:
do {
count++;
writel(0xF, &pmuC->write_sequence);
@@ -4148,7 +4150,8 @@ static void arcmsr_hardware_reset(struct
AdapterControlBlock *acb)
writel(0xD, &pmuC->write_sequence);
} while (((readl(&pmuC->host_diagnostic) &
ARCMSR_ARC1880_DiagWrite_ENABLE) == 0) && (count < 5));
writel(ARCMSR_ARC1880_RESET_ADAPTER, &pmuC->host_diagnostic);
- } else if (acb->dev_id == 0x1884) {
+ break;
+ case 0x1884:
struct MessageUnit_E __iomem *pmuE = acb->pmuE;
do {
count++;
@@ -4161,10 +4164,13 @@ static void arcmsr_hardware_reset(struct
AdapterControlBlock *acb)
} while (((readl(&pmuE->host_diagnostic_3xxx) &
ARCMSR_ARC1884_DiagWrite_ENABLE) == 0) && (count < 5));
writel(ARCMSR_ARC188X_RESET_ADAPTER,
&pmuE->host_diagnostic_3xxx);
- } else if ((acb->dev_id == 0x1214)) {
+ break;
+ case 0x1214:
writel(0x20, pmuD->reset_request);
- } else {
+ break;
+ default:
pci_write_config_byte(acb->pdev, 0x84, 0x20);
+ break;
}
msleep(2000);
/* write back pci config data */
Bad MAINTAINERS pattern in section 'CXLFLASH (IBM Coherent Accelerator Processor Interface CAPI Flash) SCSI DRIVER'
Please fix this defect appropriately. linux-next MAINTAINERS section: 3964CXLFLASH (IBM Coherent Accelerator Processor Interface CAPI Flash) SCSI DRIVER 3965M: Manoj N. Kumar 3966M: Matthew R. Ochs 3967M: Uma Krishnan 3968L: linux-scsi@vger.kernel.org 3969S: Supported 3970F: drivers/scsi/cxlflash/ --> 3971F: include/uapi/scsi/cxlflash_ioctls.h 3972F: Documentation/powerpc/cxlflash.txt Commit that introduced this: commit 11f43ae7735a04994ef3c33295d386ef4e5529b7 Author: Matthew R. Ochs Date: Wed Oct 21 15:15:22 2015 -0500 MAINTAINERS: Add cxlflash driver Add stanza for cxlflash SCSI driver. Signed-off-by: Matthew R. Ochs Signed-off-by: Manoj N. Kumar Reviewed-by: Brian King Reviewed-by: Andrew Donnellan Signed-off-by: James Bottomley MAINTAINERS | 9 + 1 file changed, 9 insertions(+) No commit with include/uapi/scsi/cxlflash_ioctls.h found
[PATCH 7/7] mpt3sas: Remove unused macro MPT3SAS_FMT
All the uses have been removed, delete the macro.
Signed-off-by: Joe Perches
---
drivers/scsi/mpt3sas/mpt3sas_base.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 941a4faf20be..8f1d6b071b39 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -158,8 +158,6 @@ struct mpt3sas_nvme_cmd {
/*
* logging format
*/
-#define MPT3SAS_FMT"%s: "
-
#define ioc_err(ioc, fmt, ...) \
pr_err("%s: " fmt, (ioc)->name, ##__VA_ARGS__)
#define ioc_notice(ioc, fmt, ...) \
--
2.15.0
[PATCH 6/7] mpt3sas: Convert logging uses with MPT3SAS_FMT without logging levels
Convert these uses to ioc_ where appropriate.
Signed-off-by: Joe Perches
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 41 +++-
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 12 +-
drivers/scsi/mpt3sas/mpt3sas_transport.c | 6 ++---
3 files changed, 28 insertions(+), 31 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 386af6739867..cf5a21717a6f 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -2819,9 +2819,8 @@ _base_enable_msix(struct MPT3SAS_ADAPTER *ioc)
ioc->reply_queue_count = min_t(int, ioc->cpu_count,
ioc->msix_vector_count);
- printk(MPT3SAS_FMT "MSI-X vectors supported: %d, no of cores"
- ": %d, max_msix_vectors: %d\n", ioc->name, ioc->msix_vector_count,
- ioc->cpu_count, max_msix_vectors);
+ ioc_info(ioc, "MSI-X vectors supported: %d, no of cores: %d,
max_msix_vectors: %d\n",
+ioc->msix_vector_count, ioc->cpu_count, max_msix_vectors);
if (!ioc->rdpq_array_enable && max_msix_vectors == -1)
local_max_msix_vectors = (reset_devices) ? 1 : 8;
@@ -2886,8 +2885,7 @@ mpt3sas_base_unmap_resources(struct MPT3SAS_ADAPTER *ioc)
{
struct pci_dev *pdev = ioc->pdev;
- dexitprintk(ioc, printk(MPT3SAS_FMT "%s\n",
- ioc->name, __func__));
+ dexitprintk(ioc, ioc_info(ioc, "%s\n", __func__));
_base_free_irq(ioc);
_base_disable_msix(ioc);
@@ -3007,9 +3005,8 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
ioc->combined_reply_index_count,
sizeof(resource_size_t *), GFP_KERNEL);
if (!ioc->replyPostRegisterIndex) {
- dfailprintk(ioc, printk(MPT3SAS_FMT
- "allocation for reply Post Register Index failed!!!\n",
- ioc->name));
+ dfailprintk(ioc,
+ ioc_warn(ioc, "allocation for reply Post
Register Index failed!!!\n"));
r = -ENOMEM;
goto out_fail;
}
@@ -5438,26 +5435,26 @@ _base_wait_for_iocstate(struct MPT3SAS_ADAPTER *ioc,
int timeout)
u32 ioc_state;
int rc;
- dinitprintk(ioc, printk(MPT3SAS_FMT "%s\n", ioc->name,
- __func__));
+ dinitprintk(ioc, ioc_info(ioc, "%s\n", __func__));
if (ioc->pci_error_recovery) {
- dfailprintk(ioc, printk(MPT3SAS_FMT
- "%s: host in pci error recovery\n", ioc->name, __func__));
+ dfailprintk(ioc,
+ ioc_info(ioc, "%s: host in pci error recovery\n",
+__func__));
return -EFAULT;
}
ioc_state = mpt3sas_base_get_iocstate(ioc, 0);
- dhsprintk(ioc, printk(MPT3SAS_FMT "%s: ioc_state(0x%08x)\n",
- ioc->name, __func__, ioc_state));
+ dhsprintk(ioc,
+ ioc_info(ioc, "%s: ioc_state(0x%08x)\n",
+ __func__, ioc_state));
if (((ioc_state & MPI2_IOC_STATE_MASK) == MPI2_IOC_STATE_READY) ||
(ioc_state & MPI2_IOC_STATE_MASK) == MPI2_IOC_STATE_OPERATIONAL)
return 0;
if (ioc_state & MPI2_DOORBELL_USED) {
- dhsprintk(ioc, printk(MPT3SAS_FMT
- "unexpected doorbell active!\n", ioc->name));
+ dhsprintk(ioc, ioc_info(ioc, "unexpected doorbell active!\n"));
goto issue_diag_reset;
}
@@ -5469,9 +5466,9 @@ _base_wait_for_iocstate(struct MPT3SAS_ADAPTER *ioc, int
timeout)
ioc_state = _base_wait_on_iocstate(ioc, MPI2_IOC_STATE_READY, timeout);
if (ioc_state) {
- dfailprintk(ioc, printk(MPT3SAS_FMT
- "%s: failed going to ready state (ioc_state=0x%x)\n",
- ioc->name, __func__, ioc_state));
+ dfailprintk(ioc,
+ ioc_info(ioc, "%s: failed going to ready state
(ioc_state=0x%x)\n",
+__func__, ioc_state));
return -EFAULT;
}
@@ -5498,9 +5495,9 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc)
r = _base_wait_for_iocstate(ioc, 10);
if (r) {
- dfailprintk(ioc, printk(MPT3SAS_FMT
- "%s: failed getting to correct state\n",
- ioc->name, __func__));
+ dfailprintk(ioc,
+ ioc_info(ioc, "%s: failed getting to correct
state\n",
+
[PATCH 3/7] mpt3sas: Convert mlsleading uses of pr_ with MPT3SAS_FMT
These have misordered uses of __func__ and ioc->name that could
mismatch MPT3SAS_FMT and "%s: ".
Convert them to ioc_.
Signed-off-by: Joe Perches
---
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 50 +++-
drivers/scsi/mpt3sas/mpt3sas_transport.c | 18
2 files changed, 29 insertions(+), 39 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 3331eba4b78d..8089be381c72 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2627,15 +2627,13 @@ mpt3sas_scsih_issue_tm(struct MPT3SAS_ADAPTER *ioc, u16
handle, u64 lun,
lockdep_assert_held(&ioc->tm_cmds.mutex);
if (ioc->tm_cmds.status != MPT3_CMD_NOT_USED) {
- pr_info(MPT3SAS_FMT "%s: tm_cmd busy!!!\n",
- __func__, ioc->name);
+ ioc_info(ioc, "%s: tm_cmd busy!!!\n", __func__);
return FAILED;
}
if (ioc->shost_recovery || ioc->remove_host ||
ioc->pci_error_recovery) {
- pr_info(MPT3SAS_FMT "%s: host reset in progress!\n",
- __func__, ioc->name);
+ ioc_info(ioc, "%s: host reset in progress!\n", __func__);
return FAILED;
}
@@ -3550,18 +3548,16 @@ _scsih_tm_tr_send(struct MPT3SAS_ADAPTER *ioc, u16
handle)
u8 tr_method = 0;
if (ioc->pci_error_recovery) {
- dewtprintk(ioc, pr_info(MPT3SAS_FMT
- "%s: host in pci error recovery: handle(0x%04x)\n",
- __func__, ioc->name,
- handle));
+ dewtprintk(ioc,
+ ioc_info(ioc, "%s: host in pci error recovery:
handle(0x%04x)\n",
+ __func__, handle));
return;
}
ioc_state = mpt3sas_base_get_iocstate(ioc, 1);
if (ioc_state != MPI2_IOC_STATE_OPERATIONAL) {
- dewtprintk(ioc, pr_info(MPT3SAS_FMT
- "%s: host is not operational: handle(0x%04x)\n",
- __func__, ioc->name,
- handle));
+ dewtprintk(ioc,
+ ioc_info(ioc, "%s: host is not operational:
handle(0x%04x)\n",
+ __func__, handle));
return;
}
@@ -3811,9 +3807,9 @@ _scsih_tm_tr_volume_send(struct MPT3SAS_ADAPTER *ioc, u16
handle)
struct _tr_list *delayed_tr;
if (ioc->pci_error_recovery) {
- dewtprintk(ioc, pr_info(MPT3SAS_FMT
- "%s: host reset in progress!\n",
- __func__, ioc->name));
+ dewtprintk(ioc,
+ ioc_info(ioc, "%s: host reset in progress!\n",
+ __func__));
return;
}
@@ -3863,9 +3859,9 @@ _scsih_tm_volume_tr_complete(struct MPT3SAS_ADAPTER *ioc,
u16 smid,
mpt3sas_base_get_reply_virt_addr(ioc, reply);
if (ioc->shost_recovery || ioc->pci_error_recovery) {
- dewtprintk(ioc, pr_info(MPT3SAS_FMT
- "%s: host reset in progress!\n",
- __func__, ioc->name));
+ dewtprintk(ioc,
+ ioc_info(ioc, "%s: host reset in progress!\n",
+ __func__));
return 1;
}
if (unlikely(!mpi_reply)) {
@@ -3950,21 +3946,21 @@ _scsih_issue_delayed_sas_io_unit_ctrl(struct
MPT3SAS_ADAPTER *ioc,
unsigned long flags;
if (ioc->remove_host) {
- dewtprintk(ioc, pr_info(MPT3SAS_FMT
- "%s: host has been removed\n",
-__func__, ioc->name));
+ dewtprintk(ioc,
+ ioc_info(ioc, "%s: host has been removed\n",
+ __func__));
return;
} else if (ioc->pci_error_recovery) {
- dewtprintk(ioc, pr_info(MPT3SAS_FMT
- "%s: host in pci error recovery\n",
- __func__, ioc->name));
+ dewtprintk(ioc,
+ ioc_info(ioc, "%s: host in pci error recovery\n",
+ __func__));
return;
}
ioc_state = mpt3sas_base_get_iocstate(ioc, 1);
if (ioc_state != MPI2_IOC_STATE_OPERATIONAL) {
- dewtprintk(ioc, pr_info(MPT3SAS_FMT
- "%s: host is not operational\n",
- __func__, ioc->name));
+ dewtprintk(ioc,
+ ioc_info(ioc, "%s: hos
[PATCH 1/7] mpt3sas: Add ioc_ logging macros
These macros can help identify specific logging uses and eventually
perhaps reduce object sizes.
Signed-off-by: Joe Perches
---
drivers/scsi/mpt3sas/mpt3sas_base.h | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h
b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 96dc15e90bd8..941a4faf20be 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -160,6 +160,15 @@ struct mpt3sas_nvme_cmd {
*/
#define MPT3SAS_FMT"%s: "
+#define ioc_err(ioc, fmt, ...) \
+ pr_err("%s: " fmt, (ioc)->name, ##__VA_ARGS__)
+#define ioc_notice(ioc, fmt, ...) \
+ pr_notice("%s: " fmt, (ioc)->name, ##__VA_ARGS__)
+#define ioc_warn(ioc, fmt, ...)
\
+ pr_warn("%s: " fmt, (ioc)->name, ##__VA_ARGS__)
+#define ioc_info(ioc, fmt, ...)
\
+ pr_info("%s: " fmt, (ioc)->name, ##__VA_ARGS__)
+
/*
* WarpDrive Specific Log codes
*/
--
2.15.0
[PATCH 0/7] Neaten logging uses
Several defects exist in the logging uses o Missing KERN_ o Unnecessary KERN_ uses with panic o Mismatched MPT3SAS_FMT and %s: with name and __func__ Correct these defects and perhaps add some clarity to the logging. Joe Perches (7): mpt3sas: Add ioc_ logging macros mpt3sas: Convert uses of pr_ with MPT3SAS_FMT to ioc_ mpt3sas: Convert mlsleading uses of pr_ with MPT3SAS_FMT mpt3sas: Convert logging uses with MPT3SAS_FMT and reply_q_name to %s: mpt3sas: Remove KERN_WARNING from panic uses mpt3sas: Convert logging uses with MPT3SAS_FMT without logging levels mpt3sas: Remove unused macro MPT3SAS_FMT drivers/scsi/mpt3sas/mpt3sas_base.c | 1116 +--- drivers/scsi/mpt3sas/mpt3sas_base.h |9 +- drivers/scsi/mpt3sas/mpt3sas_config.c | 89 +- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 493 - drivers/scsi/mpt3sas/mpt3sas_scsih.c| 1487 --- drivers/scsi/mpt3sas/mpt3sas_transport.c| 337 +++--- drivers/scsi/mpt3sas/mpt3sas_trigger_diag.c | 101 +- drivers/scsi/mpt3sas/mpt3sas_warpdrive.c| 70 +- 8 files changed, 1618 insertions(+), 2084 deletions(-) -- 2.15.0
[PATCH 4/7] mpt3sas: Convert logging uses with MPT3SAS_FMT and reply_q_name to %s:
Convert the existing 2 uses to make the format and arguments
matching more obvious.
Miscellanea:
o Move the word "enabled" into the format to trivially reduce object size
o Remove unnecessary parentheses
Signed-off-by: Joe Perches
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 5c6634b7ca74..386af6739867 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -2705,7 +2705,7 @@ _base_request_irq(struct MPT3SAS_ADAPTER *ioc, u8 index)
r = request_irq(pci_irq_vector(pdev, index), _base_interrupt,
IRQF_SHARED, reply_q->name, reply_q);
if (r) {
- pr_err(MPT3SAS_FMT "unable to allocate interrupt %d!\n",
+ pr_err("%s: unable to allocate interrupt %d!\n",
reply_q->name, pci_irq_vector(pdev, index));
kfree(reply_q);
return -EBUSY;
@@ -3034,10 +3034,10 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
}
list_for_each_entry(reply_q, &ioc->reply_queue_list, list)
- pr_info(MPT3SAS_FMT "%s: IRQ %d\n",
- reply_q->name, ((ioc->msix_enable) ? "PCI-MSI-X enabled" :
- "IO-APIC enabled"),
- pci_irq_vector(ioc->pdev, reply_q->msix_index));
+ pr_info("%s: %s enabled: IRQ %d\n",
+ reply_q->name,
+ ioc->msix_enable ? "PCI-MSI-X" : "IO-APIC",
+ pci_irq_vector(ioc->pdev, reply_q->msix_index));
ioc_info(ioc, "iomem(%pap), mapped(0x%p), size(%d)\n",
&chip_phys, ioc->chip, memap_sz);
--
2.15.0
[PATCH 5/7] mpt3sas: Remove KERN_WARNING from panic uses
Remove the logging level as panic calls stop the machine and should
always be emitted regardless of requested logging level.
These existing panic uses are perhaps inappropriate.
Miscellanea:
o Coalesce formats and convert MPT3SAS_FMT to "%s: " to improve clarity
Signed-off-by: Joe Perches
---
drivers/scsi/mpt3sas/mpt3sas_config.c | 41 +--
1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c
b/drivers/scsi/mpt3sas/mpt3sas_config.c
index 38d3b163b5d1..02209447f4ef 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_config.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
@@ -421,12 +421,10 @@ _config_request(struct MPT3SAS_ADAPTER *ioc,
Mpi2ConfigRequest_t
(mpi_reply->Header.PageType & 0xF)) {
_debug_dump_mf(mpi_request, ioc->request_sz/4);
_debug_dump_reply(mpi_reply, ioc->request_sz/4);
- panic(KERN_WARNING MPT3SAS_FMT "%s: Firmware BUG:" \
- " mpi_reply mismatch: Requested PageType(0x%02x)" \
- " Reply PageType(0x%02x)\n", \
- ioc->name, __func__,
- (mpi_request->Header.PageType & 0xF),
- (mpi_reply->Header.PageType & 0xF));
+ panic("%s: %s: Firmware BUG: mpi_reply mismatch:
Requested PageType(0x%02x) Reply PageType(0x%02x)\n",
+ ioc->name, __func__,
+ mpi_request->Header.PageType & 0xF,
+ mpi_reply->Header.PageType & 0xF);
}
if (((mpi_request->Header.PageType & 0xF) ==
@@ -434,11 +432,10 @@ _config_request(struct MPT3SAS_ADAPTER *ioc,
Mpi2ConfigRequest_t
mpi_request->ExtPageType != mpi_reply->ExtPageType) {
_debug_dump_mf(mpi_request, ioc->request_sz/4);
_debug_dump_reply(mpi_reply, ioc->request_sz/4);
- panic(KERN_WARNING MPT3SAS_FMT "%s: Firmware BUG:" \
- " mpi_reply mismatch: Requested ExtPageType(0x%02x)"
- " Reply ExtPageType(0x%02x)\n",
- ioc->name, __func__, mpi_request->ExtPageType,
- mpi_reply->ExtPageType);
+ panic("%s: %s: Firmware BUG: mpi_reply mismatch:
Requested ExtPageType(0x%02x) Reply ExtPageType(0x%02x)\n",
+ ioc->name, __func__,
+ mpi_request->ExtPageType,
+ mpi_reply->ExtPageType);
}
ioc_status = le16_to_cpu(mpi_reply->IOCStatus)
& MPI2_IOCSTATUS_MASK;
@@ -461,14 +458,10 @@ _config_request(struct MPT3SAS_ADAPTER *ioc,
Mpi2ConfigRequest_t
_debug_dump_reply(mpi_reply, ioc->request_sz/4);
_debug_dump_config(p, min_t(u16, mem.sz,
config_page_sz)/4);
- panic(KERN_WARNING MPT3SAS_FMT
- "%s: Firmware BUG:" \
- " config page mismatch:"
- " Requested PageType(0x%02x)"
- " Reply PageType(0x%02x)\n",
- ioc->name, __func__,
- (mpi_request->Header.PageType & 0xF),
- (p[3] & 0xF));
+ panic("%s: %s: Firmware BUG: config page
mismatch: Requested PageType(0x%02x) Reply PageType(0x%02x)\n",
+ ioc->name, __func__,
+ mpi_request->Header.PageType & 0xF,
+ p[3] & 0xF);
}
if (((mpi_request->Header.PageType & 0xF) ==
@@ -478,13 +471,9 @@ _config_request(struct MPT3SAS_ADAPTER *ioc,
Mpi2ConfigRequest_t
_debug_dump_reply(mpi_reply, ioc->request_sz/4);
_debug_dump_config(p, min_t(u16, mem.sz,
config_page_sz)/4);
- panic(KERN_WARNING MPT3SAS_FMT
- "%s: Firmware BUG:" \
- " config page mismatch:"
- " Requested ExtPageType(0x%02x)"
- " Reply ExtPageType(0x%02x)\n",
-
Re: [PATCH] scsi: mpt3sas: make sysfs error messages ratelimited
On Fri, 2018-09-07 at 11:39 +0100, Colin King wrote:
> From: Colin Ian King
>
> It is possible to heavily spam the kernel logs with messages by
> excessive reading of the mpt3sas sysfs files. Make the error messages
> ratelimited to reduce the spamming effect.
[]
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
[]
> @@ -2958,7 +2958,7 @@ _ctl_BRM_status_show(struct device *cdev, struct
> device_attribute *attr,
> ssize_t rc = 0;
>
> if (!ioc->is_warpdrive) {
> - pr_err(MPT3SAS_FMT "%s: BRM attribute is only for"
> + pr_err_ratelimited(MPT3SAS_FMT "%s: BRM attribute is only for"
> " warpdrive\n", ioc->name, __func__);
> goto out;
> }
trivia:
All the uses of MPT3SAS_FMT in drivers/scsi/mpt3sas/ seem
not nice. It obfuscate the format and arguments.
Perhaps better would be create ioc_ macros.
[trivial PATCH V2] treewide: Align function definition open/close braces
Some functions definitions have either the initial open brace and/or
the closing brace outside of column 1.
Move those braces to column 1.
This allows various function analyzers like gnu complexity to work
properly for these modified functions.
Signed-off-by: Joe Perches
Acked-by: Andy Shevchenko
Acked-by: Paul Moore
Acked-by: Alex Deucher
Acked-by: Dave Chinner
Reviewed-by: Darrick J. Wong
Acked-by: Alexandre Belloni
Acked-by: Martin K. Petersen
Acked-by: Takashi Iwai
Acked-by: Mauro Carvalho Chehab
---
git diff -w still shows no difference.
This patch was sent but December and not applied.
As the trivial maintainer seems not active, it'd be nice if
Andrew Morton picks this up.
V2: Remove fs/xfs/libxfs/xfs_alloc.c as it's updated and remerge the rest
arch/x86/include/asm/atomic64_32.h | 2 +-
drivers/acpi/custom_method.c | 2 +-
drivers/acpi/fan.c | 2 +-
drivers/gpu/drm/amd/display/dc/core/dc.c | 2 +-
drivers/media/i2c/msp3400-kthreads.c | 2 +-
drivers/message/fusion/mptsas.c | 2 +-
drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c | 2 +-
drivers/net/wireless/ath/ath9k/xmit.c| 2 +-
drivers/platform/x86/eeepc-laptop.c | 2 +-
drivers/rtc/rtc-ab-b5ze-s3.c | 2 +-
drivers/scsi/dpt_i2o.c | 2 +-
drivers/scsi/sym53c8xx_2/sym_glue.c | 2 +-
fs/locks.c | 2 +-
fs/ocfs2/stack_user.c| 2 +-
fs/xfs/xfs_export.c | 2 +-
kernel/audit.c | 6 +++---
kernel/trace/trace_printk.c | 4 ++--
lib/raid6/sse2.c | 14 +++---
sound/soc/fsl/fsl_dma.c | 2 +-
19 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/arch/x86/include/asm/atomic64_32.h
b/arch/x86/include/asm/atomic64_32.h
index 46e1ef17d92d..92212bf0484f 100644
--- a/arch/x86/include/asm/atomic64_32.h
+++ b/arch/x86/include/asm/atomic64_32.h
@@ -123,7 +123,7 @@ static inline long long arch_atomic64_read(const atomic64_t
*v)
long long r;
alternative_atomic64(read, "=&A" (r), "c" (v) : "memory");
return r;
- }
+}
/**
* arch_atomic64_add_return - add and return
diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index b33fba70ec51..a07fbe999eb6 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -97,7 +97,7 @@ static void __exit acpi_custom_method_exit(void)
{
if (cm_dentry)
debugfs_remove(cm_dentry);
- }
+}
module_init(acpi_custom_method_init);
module_exit(acpi_custom_method_exit);
diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
index 6cf4988206f2..3563103590c6 100644
--- a/drivers/acpi/fan.c
+++ b/drivers/acpi/fan.c
@@ -219,7 +219,7 @@ fan_set_cur_state(struct thermal_cooling_device *cdev,
unsigned long state)
return fan_set_state_acpi4(device, state);
else
return fan_set_state(device, state);
- }
+}
static const struct thermal_cooling_device_ops fan_cooling_ops = {
.get_max_state = fan_get_max_state,
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 8394d69b963f..e934326a95d3 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -588,7 +588,7 @@ static void disable_dangling_plane(struct dc *dc, struct
dc_state *context)
**/
struct dc *dc_create(const struct dc_init_data *init_params)
- {
+{
struct dc *dc = kzalloc(sizeof(*dc), GFP_KERNEL);
unsigned int full_pipe_count;
diff --git a/drivers/media/i2c/msp3400-kthreads.c
b/drivers/media/i2c/msp3400-kthreads.c
index 4dd01e9f553b..dc6cb8d475b3 100644
--- a/drivers/media/i2c/msp3400-kthreads.c
+++ b/drivers/media/i2c/msp3400-kthreads.c
@@ -885,7 +885,7 @@ static int msp34xxg_modus(struct i2c_client *client)
}
static void msp34xxg_set_source(struct i2c_client *client, u16 reg, int in)
- {
+{
struct msp_state *state = to_state(i2c_get_clientdata(client));
int source, matrix;
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 439ee9c5f535..231f3a1e27bf 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -2967,7 +2967,7 @@ mptsas_exp_repmanufacture_info(MPT_ADAPTER *ioc,
mutex_unlock(&ioc->sas_mgmt.mutex);
out:
return ret;
- }
+}
static void
mptsas_parse_device_info(struct sas_identify *identify,
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
b/drivers/net/ethernet
Re: [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
On Wed, 2017-12-20 at 10:59 +0100, Greg Kroah-Hartman wrote:
> > > Why you didn't send that patch to the sysfs maintainer is a bit odd... :)
> >
> > So here's an opportunity for you:
> >
> > The sysfs maintainer hasn't added include/linux/sysfs.h to
> > the list of maintained files...
> >
> > DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS
> > M: Greg Kroah-Hartman
> > T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
> > S: Supported
> > F: Documentation/kobject.txt
> > F: drivers/base/
> > F: fs/debugfs/
> > F: fs/sysfs/
> > F: include/linux/debugfs.h
> > F: include/linux/kobj*
> > F: lib/kobj*
>
> Heh, good point, but using get_maintainer.pl does put me at the top of
> the list that you should be cc:ing:
>
> $ ./scripts/get_maintainer.pl --file include/linux/sysfs.h
> Greg Kroah-Hartman
> (commit_signer:3/3=100%,authored:2/3=67%,added_lines:7/8=88%)
> Kate Stewart (commit_signer:1/3=33%)
> Thomas Gleixner (commit_signer:1/3=33%)
> Philippe Ombredanne (commit_signer:1/3=33%)
> Nick Desaulniers
> (commit_signer:1/3=33%,authored:1/3=33%,added_lines:1/8=12%,removed_lines:1/1=100%)
> linux-ker...@vger.kernel.org (open list)
The script I use to send patches adds --nogit --nogit-fallback
to copy only listed maintainers because people that send cleanup
patches don't generally like to get random patches.
> > btw: there are many uses of a reversed declaration style of DEVICE_ATTR
> >
> > Here's another thing that could be done for more DEVICE_ATTR_ uses.
> >
> > ===
> >
> > Some DEVICE_ATTR definitions use a reversed static function form from
> > the typical. Convert them to use the more common macro form so it is
> > easier to grep for the style.
[]
> > $ git grep --name-only -w DEVICE_ATTR | \
> > xargs perl -i dev_attr_rw_backwards.perl
> Ah, nice, I love perl :
That was a bad copy/paste of the script.
The actual script for RW is:
$ cat dev_attr_rw_backwards.perl
local $/;
while (<>) {
my $file = $_;
while ($file =~ m/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,/g) {
my $var = $1;
if ($file =~
s/\bDEVICE_ATTR\s*\(\s*${var}\s*,\s*\(?(\s*S_IRUGO\s*\|\s*S_IWUSR|\s*S_IWUSR\s*\|\s*S_IRUGO\s*|\s*0644\s*)\)?\s*,\s*show_${var}\s*,\s*store_${var}\s*\)/DEVICE_ATTR_RW(${var})/g)
{
$file =~ s/\bshow_${var}\b/${var}_show/g;
$file =~ s/\bstore_${var}\b/${var}_store/g;
}
}
print $file;
}
There are 3 different perl scripts for rw, ro, and wo.
and these scripts, because of function renaming and
possible reuse of the original function names by other
string concatenated macros, create some bad conversions
so they need some manual cleanups too.
Perhaps coccinelle could do a better job of it, but
likely string concatenation macro uses are going to
be hard to deal with in any case.
Re: [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
On Wed, 2017-12-20 at 10:32 +0100, Greg Kroah-Hartman wrote:
> On Wed, Dec 20, 2017 at 01:24:44AM -0800, Joe Perches wrote:
> > On Wed, 2017-12-20 at 10:34 +0200, Jarkko Nikula wrote:
> > > On Tue, Dec 19, 2017 at 10:15:07AM -0800, Joe Perches wrote:
> > > > Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible.
> >
> > []
> > > > diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c
> >
> > []
> > > > @@ -854,7 +854,7 @@ static ssize_t dma_op_mode_store(struct device *dev,
> > > > return size;
> > > > }
> > > >
> > > > -static DEVICE_ATTR(dma_op_mode, 0644, dma_op_mode_show,
> > > > dma_op_mode_store);
> > > > +static DEVICE_ATTR_RW(dma_op_mode);
> > > >
> > >
> > > While I can ack this part here if it helps generic cleanup effort I
> > > don't understart would it improve code readability in general? Mode 644
> > > is clear and don't need any grepping but for DEVICE_ATTR_RW() I had to go
> > > through all of these files in order to see what does it mean:
>
> Yeah, 644 is "clear", but _RW() is even more clear. Ideally I want to
> get rid of all of the "non-standard" users that set random modes of
> sysfs files, as we get it wrong too many times. Using the "defaults" is
> much better.
>
> > Are you suggesting that device.h (as that is where
> > DEVICE_ATTR and the other DEVICE_ATTR_ variants
> > are #defined) should have better comments for the
> > _ variants?
> >
> > > DEVICE_ATTR_RW: include/linux/device.h
> > > __ATTR_RW: include/linux/sysfs.h
> > > S_IWUSR: include/uapi/linux/stat.h
> > > S_IRUGO: include/linux/stat.h
> >
> > btw: patch 1/4 of the series does remove the uses of
> > S_ from these macros in sysfs.h and converts
> > them to simple octal instead.
>
> Why you didn't send that patch to the sysfs maintainer is a bit odd... :)
So here's an opportunity for you:
The sysfs maintainer hasn't added include/linux/sysfs.h to
the list of maintained files...
DRIVER CORE, KOBJECTS, DEBUGFS AND SYSFS
M: Greg Kroah-Hartman
T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
S: Supported
F: Documentation/kobject.txt
F: drivers/base/
F: fs/debugfs/
F: fs/sysfs/
F: include/linux/debugfs.h
F: include/linux/kobj*
F: lib/kobj*
> I should be taking this whole series through my tree. Joe, thanks for
> doing this in an automated way, I've been wanting to see this done for a
> long time.
btw: there are many uses of a reversed declaration style of DEVICE_ATTR
Here's another thing that could be done for more DEVICE_ATTR_ uses.
===
Some DEVICE_ATTR definitions use a reversed static function form from
the typical. Convert them to use the more common macro form so it is
easier to grep for the style.
i.e.:
static ssize_t show_(...)
and
static ssize_t store_(...)
where the static function names in the DEVICE_ATTR_RW macro are reversed
static ssize_t _show(...)
and
static ssize_t _store(...)
Done with perl script
$ cat dev_attr_rw_backwards.perl
local $/;
while (<>) {
my $file = $_;
while ($file =~ m/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,/g) {
my $var = $1;
if ($file =~ s/\bDEVICE_ATTR\s*\(\s*${var}\s*,\s*\(?(\s*S_IRUGO\s*\|\s*S
$file =~ s/\bshow_${var}\b/${var}_show/g;
$file =~ s/\bstore_${var}\b/${var}_store/g;
}
}
print $file;
}
$ git grep --name-only -w DEVICE_ATTR | \
xargs perl -i dev_attr_rw_backwards.perl
Re: [-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
On Wed, 2017-12-20 at 10:34 +0200, Jarkko Nikula wrote: > On Tue, Dec 19, 2017 at 10:15:07AM -0800, Joe Perches wrote: > > Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible. [] > > diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c [] > > @@ -854,7 +854,7 @@ static ssize_t dma_op_mode_store(struct device *dev, > > return size; > > } > > > > -static DEVICE_ATTR(dma_op_mode, 0644, dma_op_mode_show, dma_op_mode_store); > > +static DEVICE_ATTR_RW(dma_op_mode); > > > > While I can ack this part here if it helps generic cleanup effort I > don't understart would it improve code readability in general? Mode 644 > is clear and don't need any grepping but for DEVICE_ATTR_RW() I had to go > through all of these files in order to see what does it mean: Are you suggesting that device.h (as that is where DEVICE_ATTR and the other DEVICE_ATTR_ variants are #defined) should have better comments for the _ variants? > DEVICE_ATTR_RW: include/linux/device.h > __ATTR_RW: include/linux/sysfs.h > S_IWUSR: include/uapi/linux/stat.h > S_IRUGO: include/linux/stat.h btw: patch 1/4 of the series does remove the uses of S_ from these macros in sysfs.h and converts them to simple octal instead.
Re: [-next PATCH 4/4] treewide: Use DEVICE_ATTR_WO
On Tue, 2017-12-19 at 19:44 +0100, Borislav Petkov wrote:
> On Tue, Dec 19, 2017 at 10:15:09AM -0800, Joe Perches wrote:
> > Convert DEVICE_ATTR uses to DEVICE_ATTR_WO where possible.
> >
> > Done with perl script:
> >
> > $ git grep -w --name-only DEVICE_ATTR | \
> > xargs perl -i -e 'local $/; while (<>) {
> > s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(?:\s*S_IWUSR\s*|\s*0200\s*)\)?\s*,\s*NULL\s*,\s*\s_store\s*\)/DEVICE_ATTR_WO(\1)/g;
> > print;}'
[]
> > diff --git a/arch/x86/kernel/cpu/microcode/core.c
> > b/arch/x86/kernel/cpu/microcode/core.c
[]
> > @@ -560,7 +560,7 @@ static ssize_t pf_show(struct device *dev,
> > return sprintf(buf, "0x%x\n", uci->cpu_sig.pf);
> > }
> >
> > -static DEVICE_ATTR(reload, 0200, NULL, reload_store);
> > +static DEVICE_ATTR_WO(reload);
> > static DEVICE_ATTR(version, 0400, version_show, NULL);
> > static DEVICE_ATTR(processor_flags, 0400, pf_show, NULL);
> >
>
> # cat /sys/devices/system/cpu/microcode/reload
> cat: /sys/devices/system/cpu/microcode/reload: Permission denied
Not different behavior. It was and remains write only.
> The reason for the code churn being?
Consistency for easier grep by use-type.
[-next PATCH 0/4] sysfs and DEVICE_ATTR_
Joe Perches (4): sysfs.h: Use octal permissions treewide: Use DEVICE_ATTR_RW treewide: Use DEVICE_ATTR_RO treewide: Use DEVICE_ATTR_WO arch/arm/mach-pxa/sharpsl_pm.c | 4 +- arch/s390/kernel/smp.c | 2 +- arch/s390/kernel/topology.c| 3 +- arch/sh/drivers/push-switch.c | 2 +- arch/tile/kernel/sysfs.c | 12 ++-- arch/x86/kernel/cpu/microcode/core.c | 2 +- drivers/acpi/device_sysfs.c| 6 +- drivers/char/ipmi/ipmi_msghandler.c| 17 +++--- drivers/gpu/drm/i915/i915_sysfs.c | 12 ++-- drivers/input/touchscreen/elants_i2c.c | 2 +- drivers/net/ethernet/ibm/ibmvnic.c | 2 +- drivers/net/wimax/i2400m/sysfs.c | 3 +- drivers/nvme/host/core.c | 10 ++-- drivers/platform/x86/compal-laptop.c | 18 ++ drivers/s390/cio/css.c | 8 +-- drivers/s390/cio/device.c | 10 ++-- drivers/s390/crypto/ap_card.c | 2 +- drivers/scsi/hpsa.c| 10 ++-- drivers/scsi/lpfc/lpfc_attr.c | 64 -- .../staging/media/atomisp/pci/atomisp2/hmm/hmm.c | 8 +-- drivers/thermal/thermal_sysfs.c| 17 +++--- drivers/tty/serial/sh-sci.c| 2 +- drivers/usb/host/xhci-dbgcap.c | 2 +- drivers/usb/phy/phy-tahvo.c| 2 +- drivers/video/fbdev/auo_k190x.c| 4 +- drivers/video/fbdev/w100fb.c | 4 +- include/linux/sysfs.h | 14 ++--- lib/test_firmware.c| 14 ++--- lib/test_kmod.c| 14 ++--- sound/soc/omap/mcbsp.c | 4 +- sound/soc/soc-core.c | 2 +- sound/soc/soc-dapm.c | 2 +- 32 files changed, 120 insertions(+), 158 deletions(-) -- 2.15.0
[-next PATCH 4/4] treewide: Use DEVICE_ATTR_WO
Convert DEVICE_ATTR uses to DEVICE_ATTR_WO where possible.
Done with perl script:
$ git grep -w --name-only DEVICE_ATTR | \
xargs perl -i -e 'local $/; while (<>) {
s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(?:\s*S_IWUSR\s*|\s*0200\s*)\)?\s*,\s*NULL\s*,\s*\s_store\s*\)/DEVICE_ATTR_WO(\1)/g;
print;}'
Signed-off-by: Joe Perches
---
arch/s390/kernel/smp.c | 2 +-
arch/x86/kernel/cpu/microcode/core.c | 2 +-
drivers/input/touchscreen/elants_i2c.c | 2 +-
drivers/net/ethernet/ibm/ibmvnic.c | 2 +-
drivers/net/wimax/i2400m/sysfs.c | 3 +--
drivers/scsi/lpfc/lpfc_attr.c | 3 +--
drivers/thermal/thermal_sysfs.c| 2 +-
7 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
index b8c1a85bcf2d..a919b2f0141d 100644
--- a/arch/s390/kernel/smp.c
+++ b/arch/s390/kernel/smp.c
@@ -1151,7 +1151,7 @@ static ssize_t __ref rescan_store(struct device *dev,
rc = smp_rescan_cpus();
return rc ? rc : count;
}
-static DEVICE_ATTR(rescan, 0200, NULL, rescan_store);
+static DEVICE_ATTR_WO(rescan);
#endif /* CONFIG_HOTPLUG_CPU */
static int __init s390_smp_init(void)
diff --git a/arch/x86/kernel/cpu/microcode/core.c
b/arch/x86/kernel/cpu/microcode/core.c
index c4fa4a85d4cb..09c74b0560dd 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -560,7 +560,7 @@ static ssize_t pf_show(struct device *dev,
return sprintf(buf, "0x%x\n", uci->cpu_sig.pf);
}
-static DEVICE_ATTR(reload, 0200, NULL, reload_store);
+static DEVICE_ATTR_WO(reload);
static DEVICE_ATTR(version, 0400, version_show, NULL);
static DEVICE_ATTR(processor_flags, 0400, pf_show, NULL);
diff --git a/drivers/input/touchscreen/elants_i2c.c
b/drivers/input/touchscreen/elants_i2c.c
index a458e5ec9e41..819213e88f32 100644
--- a/drivers/input/touchscreen/elants_i2c.c
+++ b/drivers/input/touchscreen/elants_i2c.c
@@ -1000,7 +1000,7 @@ static ssize_t show_iap_mode(struct device *dev,
"Normal" : "Recovery");
}
-static DEVICE_ATTR(calibrate, S_IWUSR, NULL, calibrate_store);
+static DEVICE_ATTR_WO(calibrate);
static DEVICE_ATTR(iap_mode, S_IRUGO, show_iap_mode, NULL);
static DEVICE_ATTR(update_fw, S_IWUSR, NULL, write_update_fw);
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
b/drivers/net/ethernet/ibm/ibmvnic.c
index 1dc4aef37d3a..42b96e1a1b13 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -4411,7 +4411,7 @@ static ssize_t failover_store(struct device *dev, struct
device_attribute *attr,
return count;
}
-static DEVICE_ATTR(failover, 0200, NULL, failover_store);
+static DEVICE_ATTR_WO(failover);
static unsigned long ibmvnic_get_desired_dma(struct vio_dev *vdev)
{
diff --git a/drivers/net/wimax/i2400m/sysfs.c b/drivers/net/wimax/i2400m/sysfs.c
index 1237109f251a..8c67df11105c 100644
--- a/drivers/net/wimax/i2400m/sysfs.c
+++ b/drivers/net/wimax/i2400m/sysfs.c
@@ -65,8 +65,7 @@ ssize_t i2400m_idle_timeout_store(struct device *dev,
}
static
-DEVICE_ATTR(i2400m_idle_timeout, S_IWUSR,
- NULL, i2400m_idle_timeout_store);
+DEVICE_ATTR_WO(i2400m_idle_timeout);
static
struct attribute *i2400m_dev_attrs[] = {
diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
index 517ff203cfde..6ddaf51a23f6 100644
--- a/drivers/scsi/lpfc/lpfc_attr.c
+++ b/drivers/scsi/lpfc/lpfc_attr.c
@@ -2418,8 +2418,7 @@ lpfc_soft_wwn_enable_store(struct device *dev, struct
device_attribute *attr,
return count;
}
-static DEVICE_ATTR(lpfc_soft_wwn_enable, S_IWUSR, NULL,
- lpfc_soft_wwn_enable_store);
+static DEVICE_ATTR_WO(lpfc_soft_wwn_enable);
/**
* lpfc_soft_wwpn_show - Return the cfg soft ww port name of the adapter
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 2bc964392924..ba81c9080f6e 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -317,7 +317,7 @@ emul_temp_store(struct device *dev, struct device_attribute
*attr,
return ret ? ret : count;
}
-static DEVICE_ATTR(emul_temp, S_IWUSR, NULL, emul_temp_store);
+static DEVICE_ATTR_WO(emul_temp);
#endif
static ssize_t
--
2.15.0
[-next PATCH 2/4] treewide: Use DEVICE_ATTR_RW
Convert DEVICE_ATTR uses to DEVICE_ATTR_RW where possible.
Done with perl script:
$ git grep -w --name-only DEVICE_ATTR | \
xargs perl -i -e 'local $/; while (<>) {
s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(\s*S_IRUGO\s*\|\s*S_IWUSR|\s*S_IWUSR\s*\|\s*S_IRUGO\s*|\s*0644\s*)\)?\s*,\s*\1_show\s*,\s*\1_store\s*\)/DEVICE_ATTR_RW(\1)/g;
print;}'
Signed-off-by: Joe Perches
---
arch/s390/kernel/topology.c | 3 +--
arch/tile/kernel/sysfs.c | 2 +-
drivers/gpu/drm/i915/i915_sysfs.c| 6 ++---
drivers/platform/x86/compal-laptop.c | 18 +--
drivers/s390/cio/device.c| 2 +-
drivers/scsi/lpfc/lpfc_attr.c| 43
drivers/thermal/thermal_sysfs.c | 9
drivers/tty/serial/sh-sci.c | 2 +-
drivers/usb/host/xhci-dbgcap.c | 2 +-
drivers/usb/phy/phy-tahvo.c | 2 +-
drivers/video/fbdev/auo_k190x.c | 4 ++--
drivers/video/fbdev/w100fb.c | 4 ++--
lib/test_firmware.c | 14 +---
lib/test_kmod.c | 14 +---
sound/soc/omap/mcbsp.c | 4 ++--
15 files changed, 49 insertions(+), 80 deletions(-)
diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
index 4d5b65e527b5..4b6e0397f66d 100644
--- a/arch/s390/kernel/topology.c
+++ b/arch/s390/kernel/topology.c
@@ -404,8 +404,7 @@ static ssize_t dispatching_store(struct device *dev,
put_online_cpus();
return rc ? rc : count;
}
-static DEVICE_ATTR(dispatching, 0644, dispatching_show,
-dispatching_store);
+static DEVICE_ATTR_RW(dispatching);
static ssize_t cpu_polarization_show(struct device *dev,
struct device_attribute *attr, char *buf)
diff --git a/arch/tile/kernel/sysfs.c b/arch/tile/kernel/sysfs.c
index 825867c53853..af5024f0fb5a 100644
--- a/arch/tile/kernel/sysfs.c
+++ b/arch/tile/kernel/sysfs.c
@@ -184,7 +184,7 @@ static ssize_t hv_stats_store(struct device *dev,
return n < 0 ? n : count;
}
-static DEVICE_ATTR(hv_stats, 0644, hv_stats_show, hv_stats_store);
+static DEVICE_ATTR_RW(hv_stats);
static int hv_stats_device_add(struct device *dev, struct subsys_interface
*sif)
{
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
b/drivers/gpu/drm/i915/i915_sysfs.c
index c74a20b80182..1d0ab8ff5915 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -447,9 +447,9 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev,
static DEVICE_ATTR(gt_act_freq_mhz, S_IRUGO, gt_act_freq_mhz_show, NULL);
static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL);
-static DEVICE_ATTR(gt_boost_freq_mhz, S_IRUGO | S_IWUSR,
gt_boost_freq_mhz_show, gt_boost_freq_mhz_store);
-static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show,
gt_max_freq_mhz_store);
-static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show,
gt_min_freq_mhz_store);
+static DEVICE_ATTR_RW(gt_boost_freq_mhz);
+static DEVICE_ATTR_RW(gt_max_freq_mhz);
+static DEVICE_ATTR_RW(gt_min_freq_mhz);
static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, vlv_rpe_freq_mhz_show, NULL);
diff --git a/drivers/platform/x86/compal-laptop.c
b/drivers/platform/x86/compal-laptop.c
index 6bcb750e1865..4f9bc72f0584 100644
--- a/drivers/platform/x86/compal-laptop.c
+++ b/drivers/platform/x86/compal-laptop.c
@@ -679,18 +679,12 @@ static int bat_writeable_property(struct power_supply
*psy,
/* == */
/* Driver Globals */
/* == */
-static DEVICE_ATTR(wake_up_pme,
- 0644, wake_up_pme_show, wake_up_pme_store);
-static DEVICE_ATTR(wake_up_modem,
- 0644, wake_up_modem_show, wake_up_modem_store);
-static DEVICE_ATTR(wake_up_lan,
- 0644, wake_up_lan_show, wake_up_lan_store);
-static DEVICE_ATTR(wake_up_wlan,
- 0644, wake_up_wlan_show,wake_up_wlan_store);
-static DEVICE_ATTR(wake_up_key,
- 0644, wake_up_key_show, wake_up_key_store);
-static DEVICE_ATTR(wake_up_mouse,
- 0644, wake_up_mouse_show, wake_up_mouse_store);
+static DEVICE_ATTR_RW(wake_up_pme);
+static DEVICE_ATTR_RW(wake_up_modem);
+static DEVICE_ATTR_RW(wake_up_lan);
+static DEVICE_ATTR_RW(wake_up_wlan);
+static DEVICE_ATTR_RW(wake_up_key);
+static DEVICE_ATTR_RW(wake_up_mouse);
static DEVICE_ATTR(fan1_input, S_IRUGO, fan_show, NULL);
static DEVICE_ATTR(temp1_input, S_IRUGO, temp_cpu, NULL);
diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
index 75a245f38e2e..6eefb67b31f3 100644
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -600,7 +600,7 @@ static ssize_t vpm_show(struct device *dev, struct
device_attribute *attr,
static DEVICE_ATTR(devtype, 0444, devtype_show, NULL);
static DEVICE_ATTR(cutype, 0444, cutype_show, NULL);
static DEVICE_ATTR(modalias, 0444, modalias_s
[-next PATCH 3/4] treewide: Use DEVICE_ATTR_RO
Convert DEVICE_ATTR uses to DEVICE_ATTR_RO where possible.
Done with perl script:
$ git grep -w --name-only DEVICE_ATTR | \
xargs perl -i -e 'local $/; while (<>) {
s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(?:\s*S_IRUGO\s*|\s*0444\s*)\)?\s*,\s*\1_show\s*,\s*NULL\s*\)/DEVICE_ATTR_RO(\1)/g;
print;}'
Signed-off-by: Joe Perches
---
arch/arm/mach-pxa/sharpsl_pm.c | 4 ++--
arch/sh/drivers/push-switch.c| 2 +-
arch/tile/kernel/sysfs.c | 10 +-
drivers/acpi/device_sysfs.c | 6 +++---
drivers/char/ipmi/ipmi_msghandler.c | 17 -
drivers/gpu/drm/i915/i915_sysfs.c| 6 +++---
drivers/nvme/host/core.c | 10 +-
drivers/s390/cio/css.c | 8
drivers/s390/cio/device.c| 8
drivers/s390/crypto/ap_card.c| 2 +-
drivers/scsi/hpsa.c | 10 +-
drivers/scsi/lpfc/lpfc_attr.c| 18 --
drivers/staging/media/atomisp/pci/atomisp2/hmm/hmm.c | 8
drivers/thermal/thermal_sysfs.c | 6 +++---
sound/soc/soc-core.c | 2 +-
sound/soc/soc-dapm.c | 2 +-
16 files changed, 58 insertions(+), 61 deletions(-)
diff --git a/arch/arm/mach-pxa/sharpsl_pm.c b/arch/arm/mach-pxa/sharpsl_pm.c
index 398ba9ba2632..ef9fd9b759cb 100644
--- a/arch/arm/mach-pxa/sharpsl_pm.c
+++ b/arch/arm/mach-pxa/sharpsl_pm.c
@@ -802,8 +802,8 @@ static ssize_t battery_voltage_show(struct device *dev,
struct device_attribute
return sprintf(buf, "%d\n", sharpsl_pm.battstat.mainbat_voltage);
}
-static DEVICE_ATTR(battery_percentage, 0444, battery_percentage_show, NULL);
-static DEVICE_ATTR(battery_voltage, 0444, battery_voltage_show, NULL);
+static DEVICE_ATTR_RO(battery_percentage);
+static DEVICE_ATTR_RO(battery_voltage);
extern void (*apm_get_power_status)(struct apm_power_info *);
diff --git a/arch/sh/drivers/push-switch.c b/arch/sh/drivers/push-switch.c
index a17181160233..762bc5619910 100644
--- a/arch/sh/drivers/push-switch.c
+++ b/arch/sh/drivers/push-switch.c
@@ -24,7 +24,7 @@ static ssize_t switch_show(struct device *dev,
struct push_switch_platform_info *psw_info = dev->platform_data;
return sprintf(buf, "%s\n", psw_info->name);
}
-static DEVICE_ATTR(switch, S_IRUGO, switch_show, NULL);
+static DEVICE_ATTR_RO(switch);
static void switch_timer(struct timer_list *t)
{
diff --git a/arch/tile/kernel/sysfs.c b/arch/tile/kernel/sysfs.c
index af5024f0fb5a..b09456a3d77a 100644
--- a/arch/tile/kernel/sysfs.c
+++ b/arch/tile/kernel/sysfs.c
@@ -38,7 +38,7 @@ static ssize_t chip_width_show(struct device *dev,
{
return sprintf(page, "%u\n", smp_width);
}
-static DEVICE_ATTR(chip_width, 0444, chip_width_show, NULL);
+static DEVICE_ATTR_RO(chip_width);
static ssize_t chip_height_show(struct device *dev,
struct device_attribute *attr,
@@ -46,7 +46,7 @@ static ssize_t chip_height_show(struct device *dev,
{
return sprintf(page, "%u\n", smp_height);
}
-static DEVICE_ATTR(chip_height, 0444, chip_height_show, NULL);
+static DEVICE_ATTR_RO(chip_height);
static ssize_t chip_serial_show(struct device *dev,
struct device_attribute *attr,
@@ -54,7 +54,7 @@ static ssize_t chip_serial_show(struct device *dev,
{
return get_hv_confstr(page, HV_CONFSTR_CHIP_SERIAL_NUM);
}
-static DEVICE_ATTR(chip_serial, 0444, chip_serial_show, NULL);
+static DEVICE_ATTR_RO(chip_serial);
static ssize_t chip_revision_show(struct device *dev,
struct device_attribute *attr,
@@ -62,7 +62,7 @@ static ssize_t chip_revision_show(struct device *dev,
{
return get_hv_confstr(page, HV_CONFSTR_CHIP_REV);
}
-static DEVICE_ATTR(chip_revision, 0444, chip_revision_show, NULL);
+static DEVICE_ATTR_RO(chip_revision);
static ssize_t type_show(struct device *dev,
@@ -71,7 +71,7 @@ static ssize_t type_show(struct device *dev,
{
return sprintf(page, "tilera\n");
}
-static DEVICE_ATTR(type, 0444, type_show, NULL);
+static DEVICE_ATTR_RO(type);
#define HV_CONF_ATTR(name, conf) \
static ssize_t name ## _show(struct device *dev,\
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index a041689e5701..545e91420cde 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -357,7 +357,7 @@ static ssize_t real_power_state_show(struct device *dev,
return sprintf(buf, "%s\n", acpi_power_state_string(state));
}
-static DEVICE_ATTR(real_power_state, 0444, real_power_state_show, N
[trivial PATCH] treewide: Align function definition open/close braces
Some functions definitions have either the initial open brace and/or
the closing brace outside of column 1.
Move those braces to column 1.
This allows various function analyzers like gnu complexity to work
properly for these modified functions.
Miscellanea:
o Remove extra trailing ; and blank line from xfs_agf_verify
Signed-off-by: Joe Perches
---
git diff -w shows no difference other than the above 'Miscellanea'
(this is against -next, but it applies against Linus' tree
with a couple offsets)
arch/x86/include/asm/atomic64_32.h | 2 +-
drivers/acpi/custom_method.c | 2 +-
drivers/acpi/fan.c | 2 +-
drivers/gpu/drm/amd/display/dc/core/dc.c | 2 +-
drivers/media/i2c/msp3400-kthreads.c | 2 +-
drivers/message/fusion/mptsas.c | 2 +-
drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c | 2 +-
drivers/net/wireless/ath/ath9k/xmit.c| 2 +-
drivers/platform/x86/eeepc-laptop.c | 2 +-
drivers/rtc/rtc-ab-b5ze-s3.c | 2 +-
drivers/scsi/dpt_i2o.c | 2 +-
drivers/scsi/sym53c8xx_2/sym_glue.c | 2 +-
fs/locks.c | 2 +-
fs/ocfs2/stack_user.c| 2 +-
fs/xfs/libxfs/xfs_alloc.c| 5 ++---
fs/xfs/xfs_export.c | 2 +-
kernel/audit.c | 6 +++---
kernel/trace/trace_printk.c | 4 ++--
lib/raid6/sse2.c | 14 +++---
sound/soc/fsl/fsl_dma.c | 2 +-
20 files changed, 30 insertions(+), 31 deletions(-)
diff --git a/arch/x86/include/asm/atomic64_32.h
b/arch/x86/include/asm/atomic64_32.h
index 97c46b8169b7..d4d4883080fa 100644
--- a/arch/x86/include/asm/atomic64_32.h
+++ b/arch/x86/include/asm/atomic64_32.h
@@ -122,7 +122,7 @@ static inline long long atomic64_read(const atomic64_t *v)
long long r;
alternative_atomic64(read, "=&A" (r), "c" (v) : "memory");
return r;
- }
+}
/**
* atomic64_add_return - add and return
diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index c68e72414a67..e967c1173ba3 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -94,7 +94,7 @@ static void __exit acpi_custom_method_exit(void)
{
if (cm_dentry)
debugfs_remove(cm_dentry);
- }
+}
module_init(acpi_custom_method_init);
module_exit(acpi_custom_method_exit);
diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
index 6cf4988206f2..3563103590c6 100644
--- a/drivers/acpi/fan.c
+++ b/drivers/acpi/fan.c
@@ -219,7 +219,7 @@ fan_set_cur_state(struct thermal_cooling_device *cdev,
unsigned long state)
return fan_set_state_acpi4(device, state);
else
return fan_set_state(device, state);
- }
+}
static const struct thermal_cooling_device_ops fan_cooling_ops = {
.get_max_state = fan_get_max_state,
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index d1488d5ee028..1e0d1e7c5324 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -461,7 +461,7 @@ static void disable_dangling_plane(struct dc *dc, struct
dc_state *context)
**/
struct dc *dc_create(const struct dc_init_data *init_params)
- {
+{
struct dc *dc = kzalloc(sizeof(*dc), GFP_KERNEL);
unsigned int full_pipe_count;
diff --git a/drivers/media/i2c/msp3400-kthreads.c
b/drivers/media/i2c/msp3400-kthreads.c
index 4dd01e9f553b..dc6cb8d475b3 100644
--- a/drivers/media/i2c/msp3400-kthreads.c
+++ b/drivers/media/i2c/msp3400-kthreads.c
@@ -885,7 +885,7 @@ static int msp34xxg_modus(struct i2c_client *client)
}
static void msp34xxg_set_source(struct i2c_client *client, u16 reg, int in)
- {
+{
struct msp_state *state = to_state(i2c_get_clientdata(client));
int source, matrix;
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 345f6035599e..69a62d23514b 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -2968,7 +2968,7 @@ mptsas_exp_repmanufacture_info(MPT_ADAPTER *ioc,
mutex_unlock(&ioc->sas_mgmt.mutex);
out:
return ret;
- }
+}
static void
mptsas_parse_device_info(struct sas_identify *identify,
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
index 3dd973475125..0ea141ece19e 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
@@ -603,7 +603,7 @@
Re: [PATCH 01/10] arch:powerpc: return -ENOMEM on failed allocation
On Wed, 2017-09-13 at 13:02 +0530, Allen Pais wrote:
> Signed-off-by: Allen Pais
I think the changelog for this series of conversions
should show that you've validated the change by
inspecting the return call chain at each modified line.
Also, it seems you've cc'd the same mailing lists for
all of the patches modified by this series.
It would be better to individually address each patch
in the series only cc'ing the appropriate maintainers
and mailing lists.
A cover letter would be good too.
> ---
> arch/powerpc/platforms/cell/spider-pci.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/platforms/cell/spider-pci.c
> b/arch/powerpc/platforms/cell/spider-pci.c
> index d1e61e2..82aa3f7 100644
> --- a/arch/powerpc/platforms/cell/spider-pci.c
> +++ b/arch/powerpc/platforms/cell/spider-pci.c
> @@ -106,7 +106,7 @@ static int __init spiderpci_pci_setup_chip(struct
> pci_controller *phb,
> dummy_page_va = kmalloc(PAGE_SIZE, GFP_KERNEL);
> if (!dummy_page_va) {
> pr_err("SPIDERPCI-IOWA:Alloc dummy_page_va failed.\n");
> - return -1;
> + return -ENOMEM;
> }
>
> dummy_page_da = dma_map_single(phb->parent, dummy_page_va,
> @@ -137,7 +137,7 @@ int __init spiderpci_iowa_init(struct iowa_bus *bus, void
> *data)
> if (!priv) {
> pr_err("SPIDERPCI-IOWA:"
> "Can't allocate struct spiderpci_iowa_private");
> - return -1;
> + return -ENOMEM;
> }
>
> if (of_address_to_resource(np, 0, &r)) {
Re: [PATCH] scsi: remove memset before memcpy
On Wed, 2017-08-30 at 00:19 +0530, Himanshu Jha wrote: > drivers/scsi/megaraid/megaraid_sas_fusion.c I don't know if you did this with coccinelle. If so, it would be good to show the tool and script in the commit message. If not, the input script is pretty simple. $ cat memset_before_memcpy.cocci @@ expression e1; expression e2; expression e3; @@ - memset(e1, 0, e3); memcpy(e1, e2, e3); $ Adding a test to make sure e1 or e3 isn't modified before any other code uses them by doing $ cat memset_before_memcpy_2.cocci @@ expression e1; expression e2; expressi on e3; @@ - memset(e1, 0, e3); ... when != \( e1 \| e3 \) memcpy(e1, e2, e3); $ finds more cases but there may be a false positive if e1 is a passed function argument and if the operation isn't effectively atomic like below: --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c @@ -2556,7 +2556,6 @@ void br_multicast_get_stats(const struct struct br_mcast_stats tdst; int i; - memset(dest, 0, sizeof(*dest)); if (p) stats = p->mcast_stats; else where the memcpy is the last line of the function.
Re: [PATCH] lpfc: nvmet_fc: fix format string
On Sat, 2017-05-20 at 21:10 +0200, Arnd Bergmann wrote: > On Sat, May 20, 2017 at 12:28 PM, Joe Perches wrote: > > On Fri, 2017-05-19 at 10:04 +0200, Arnd Bergmann wrote: > > > The lpfc_nvmeio_data() tracing helper always takes a format string and > > > three additional arguments. > > > > No it doesn't. It takes a format and arguments. > > > > I don't disagree with the patch, just the characterization > > of the lpfc_mvmeio_data call in the commit message. > > I think my description is correct, it's just not obvious from > reading the code until you also look at the lpfc_debugfs_nvme_trc > prototype: OK, but more that's a mismatch between a function and its arguments and a different called function within it.
Re: [PATCH 1/2] scsi: nsp32: add __printf attribute to logging functions
On Sat, 2017-05-20 at 13:16 +0200, Nicolas Iooss wrote:
> nsp32_message() and nsp32_dmessage() use printf format strings in order
> to format a message. Adding __printf attributes helps to detect errors
> in such format strings at build time, like:
>
> drivers/scsi/nsp32.c:3314:23: error: format '%ld' expects argument
> of type 'long int', but argument 6 has type 'pm_message_t {aka
> struct pm_message}' [-Werror=format=]
> nsp32_msg(KERN_INFO,
> "pci-suspend: pdev=0x%p, state=%ld, slot=%s, host=0x%p",
> pdev, state, pci_name(pdev), host);
>
> Fix all format string errors which were reported by gcc.
[]
> diff --git a/drivers/scsi/nsp32.c b/drivers/scsi/nsp32.c
[]
> @@ -321,7 +323,8 @@ static struct scsi_host_template nsp32_template = {
>
> #define NSP32_DEBUG_BUF_LEN 100
>
> -static void nsp32_message(const char *func, int line, char *type, char *fmt,
> ...)
> +static __printf(4, 5)
> +void nsp32_message(const char *func, int line, char *type, const char *fmt,
> ...)
> {
> va_list args;
> char buf[NSP32_DEBUG_BUF_LEN];
These could also use vsprintf extension %pV instead of
vsnprintf to a
temporary buffer and then using "%s, "
etc...
Does anyone actually have or use these cards any longer?
Re: [PATCH] lpfc: nvmet_fc: fix format string
On Fri, 2017-05-19 at 10:04 +0200, Arnd Bergmann wrote: > The lpfc_nvmeio_data() tracing helper always takes a format string and > three additional arguments. No it doesn't. It takes a format and arguments. I don't disagree with the patch, just the characterization of the lpfc_mvmeio_data call in the commit message. > The latest caller has a format string with > only two integer arguments, causing this harmless warning: > > drivers/scsi/lpfc/lpfc_nvmet.c: In function 'lpfc_nvmet_xmt_fcp_release': > drivers/scsi/lpfc/lpfc_nvmet.c:802:25: error: too many arguments for format > [-Werror=format-extra-args] > lpfc_nvmeio_data(phba, "NVMET FCP FREE: xri x%x ste %d\n", ctxp->oxid, > > We could add a dummy argument here, but it seems reasonable to print > the 'abort' flag as the third argument.
[PATCH] scsi: fas216: Add __printf validation, fix fallout
__printf makes the compiler check format and arguments.
Fix fallout.
Miscellanea:
o Convert formats to const char *
o Use vsprintf extension %pV instead of a static buffer.
o Add newline to logging and remove now unnecessary printk("\n")
o Use pr_cont where appropriate
Signed-off-by: Joe Perches
---
drivers/scsi/arm/fas216.c | 39 ---
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/drivers/scsi/arm/fas216.c b/drivers/scsi/arm/fas216.c
index 24388795ee9a..112bec886192 100644
--- a/drivers/scsi/arm/fas216.c
+++ b/drivers/scsi/arm/fas216.c
@@ -289,17 +289,20 @@ static char fas216_target(FAS216_Info *info)
return 'H';
}
-static void
-fas216_do_log(FAS216_Info *info, char target, char *fmt, va_list ap)
+__printf(3, 0) static void
+fas216_do_log(FAS216_Info *info, char target, const char *fmt, va_list ap)
{
- static char buf[1024];
+ struct va_format vaf;
+
+ vaf.fmt = fmt;
+ vaf.va = ≈
- vsnprintf(buf, sizeof(buf), fmt, ap);
- printk("scsi%d.%c: %s", info->host->host_no, target, buf);
+ printk("scsi%d.%c: %pV\n", info->host->host_no, target, &vaf);
}
+__printf(4, 5)
static void fas216_log_command(FAS216_Info *info, int level,
- struct scsi_cmnd *SCpnt, char *fmt, ...)
+ struct scsi_cmnd *SCpnt, const char *fmt, ...)
{
va_list args;
@@ -313,8 +316,9 @@ static void fas216_log_command(FAS216_Info *info, int level,
scsi_print_command(SCpnt);
}
-static void
-fas216_log_target(FAS216_Info *info, int level, int target, char *fmt, ...)
+__printf(4, 5) static void
+fas216_log_target(FAS216_Info *info, int level, int target,
+ const char *fmt, ...)
{
va_list args;
@@ -329,11 +333,10 @@ fas216_log_target(FAS216_Info *info, int level, int
target, char *fmt, ...)
va_start(args, fmt);
fas216_do_log(info, target, fmt, args);
va_end(args);
-
- printk("\n");
}
-static void fas216_log(FAS216_Info *info, int level, char *fmt, ...)
+__printf(3, 4)
+static void fas216_log(FAS216_Info *info, int level, const char *fmt, ...)
{
va_list args;
@@ -343,8 +346,6 @@ static void fas216_log(FAS216_Info *info, int level, char
*fmt, ...)
va_start(args, fmt);
fas216_do_log(info, fas216_target(info), fmt, args);
va_end(args);
-
- printk("\n");
}
#define PH_SIZE32
@@ -431,7 +432,7 @@ fas216_get_last_msg(FAS216_Info *info, int pos)
}
fas216_log(info, LOG_MESSAGES,
- "Message: %04x found at position %02x\n", packed_msg, pos);
+ "Message: %04x found at position %02x", packed_msg, pos);
return packed_msg;
}
@@ -725,7 +726,7 @@ static void fas216_cleanuptransfer(FAS216_Info *info)
fifo = fas216_readb(info, REG_CFIS) & CFIS_CF;
fas216_log(info, LOG_BUFFER, "cleaning up from previous "
- "transfer: length 0x%06x, residual 0x%x, fifo %d",
+ "transfer: length 0x%06lx, residual 0x%lx, fifo %ld",
total, residual, fifo);
/*
@@ -1144,8 +1145,8 @@ static void fas216_parse_message(FAS216_Info *info,
unsigned char *message, int
fas216_log(info, 0, "unrecognised message, rejecting");
printk("scsi%d.%c: message was", info->host->host_no,
fas216_target(info));
for (i = 0; i < msglen; i++)
- printk("%s%02X", i & 31 ? " " : "\n ", message[i]);
- printk("\n");
+ pr_cont("%s%02X", i & 31 ? " " : "\n ", message[i]);
+ pr_cont("\n");
/*
* Something strange seems to be happening here -
@@ -1582,7 +1583,7 @@ static void fas216_funcdone_intr(FAS216_Info *info,
unsigned int stat, unsigned
default:
fas216_log(info, 0, "internal phase %s for function done?"
" What do I do with this?",
- fas216_target(info), fas216_drv_phase(info));
+ fas216_drv_phase(info));
}
}
@@ -1642,7 +1643,7 @@ irqreturn_t fas216_intr(FAS216_Info *info)
fas216_bus_reset(info);
scsi_report_bus_reset(info->host, 0);
} else if (inst & INST_ILLEGALCMD) {
- fas216_log(info, LOG_ERROR, "illegal command given\n");
+ fas216_log(info, LOG_ERROR, "illegal command given");
fas216_dumpstate(info);
print_debug_list();
} else if (inst & INST_DISCONNECT)
--
2.10.0.rc2.1.g053435c
Re: Checking error messages for failed memory allocations
On Wed, 2017-04-26 at 20:50 +0200, SF Markus Elfring wrote: > > Basically most everything that has a gfp_t argument does a > > dump_stack() on OOM unless __GFP_NOWARN is specified by that gfp_t. > > How do you think about to improve any programming interface documentation > around such a function property? Feel free to submit documentation patches. > Are there any special checks needed for function implementations > which can pass the flag “__GFP_NOWARN”? No.
Re: [PATCH 2/3] scsi: ufs: Delete an error message for a failed memory allocation in ufshcd_memory_alloc()
On Wed, 2017-04-26 at 10:57 -0700, Subhash Jadavani wrote: > PS: ufshcd_memory_alloc() also does some DMA coherent memory allocation > (via dmam_alloc_coherent() APIs) and tries to print out the message on > allocation failure. Although i don't know "out of memory" messages will > be printed out by dmam_alloc_coherent() APIs or not. If it does print it > out then we might want to remove our local memory allocation failure log > messages. Basically most everything that has a gfp_t argument does a dump_stack() on OOM unless __GFP_NOWARN is specified by that gfp_t.
Re: [PATCH] scsi: qedf: Fix defective logging format and argument mismatches
On Mon, 2017-03-06 at 10:17 -0800, Joe Perches wrote: > On Tue, 2017-03-07 at 02:03 +0800, kbuild test robot wrote: > > Hi Joe, > > Hi again Fengguang's robot. Rehi. OK, it is a new message. Patch sent.
[PATCH] scsi: qedf: Use vsprintf extension %pad
Using %llx for a dma_addr_t can lead to format/argument mismatches. Use %pad and the address of the dma_addr_t instead. Signed-off-by: Joe Perches --- drivers/scsi/qedf/qedf_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c index d9d7a86b5f8b..8e2a160490e6 100644 --- a/drivers/scsi/qedf/qedf_main.c +++ b/drivers/scsi/qedf/qedf_main.c @@ -2456,8 +2456,8 @@ static int qedf_alloc_bdq(struct qedf_ctx *qedf) } QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_DISC, - "BDQ PBL addr=0x%p dma=0x%llx.\n", qedf->bdq_pbl, - qedf->bdq_pbl_dma); + "BDQ PBL addr=0x%p dma=%pad\n", + qedf->bdq_pbl, &qedf->bdq_pbl_dma); /* * Populate BDQ PBL with physical and virtual address of individual -- 2.10.0.rc2.1.g053435c
Re: [PATCH] scsi: qedf: Fix defective logging format and argument mismatches
On Tue, 2017-03-07 at 02:03 +0800, kbuild test robot wrote: > Hi Joe, Hi again Fengguang's robot. > [auto build test WARNING on scsi/for-next] > [also build test WARNING on v4.11-rc1 next-20170306] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Joe-Perches/scsi-qedf-Fix-defective-logging-format-and-argument-mismatches/20170307-005400 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next > config: parisc-allmodconfig (attached as .config) > compiler: hppa-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 Not new messages.
[PATCH] scsi: qedf: Fix defective logging format and argument mismatches
Add __printf compiler verification of format and arguments.
Fix fallout.
Signed-off-by: Joe Perches
---
drivers/scsi/qedf/qedf_dbg.h | 13 -
drivers/scsi/qedf/qedf_fip.c | 2 +-
drivers/scsi/qedf/qedf_io.c | 4 ++--
3 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/qedf/qedf_dbg.h b/drivers/scsi/qedf/qedf_dbg.h
index 23bd70628a2f..7d173f48a81e 100644
--- a/drivers/scsi/qedf/qedf_dbg.h
+++ b/drivers/scsi/qedf/qedf_dbg.h
@@ -81,14 +81,17 @@ struct qedf_dbg_ctx {
#define QEDF_INFO(pdev, level, fmt, ...) \
qedf_dbg_info(pdev, __func__, __LINE__, level, fmt, \
## __VA_ARGS__)
-
-extern void qedf_dbg_err(struct qedf_dbg_ctx *qedf, const char *func, u32 line,
+__printf(4, 5)
+void qedf_dbg_err(struct qedf_dbg_ctx *qedf, const char *func, u32 line,
const char *fmt, ...);
-extern void qedf_dbg_warn(struct qedf_dbg_ctx *qedf, const char *func, u32
line,
+__printf(4, 5)
+void qedf_dbg_warn(struct qedf_dbg_ctx *qedf, const char *func, u32 line,
const char *, ...);
-extern void qedf_dbg_notice(struct qedf_dbg_ctx *qedf, const char *func,
+__printf(4, 5)
+void qedf_dbg_notice(struct qedf_dbg_ctx *qedf, const char *func,
u32 line, const char *, ...);
-extern void qedf_dbg_info(struct qedf_dbg_ctx *qedf, const char *func, u32
line,
+__printf(5, 6)
+void qedf_dbg_info(struct qedf_dbg_ctx *qedf, const char *func, u32 line,
u32 info, const char *fmt, ...);
/* GRC Dump related defines */
diff --git a/drivers/scsi/qedf/qedf_fip.c b/drivers/scsi/qedf/qedf_fip.c
index 868d423380d1..ed58b9104f58 100644
--- a/drivers/scsi/qedf/qedf_fip.c
+++ b/drivers/scsi/qedf/qedf_fip.c
@@ -203,7 +203,7 @@ void qedf_fip_recv(struct qedf_ctx *qedf, struct sk_buff
*skb)
case FIP_DT_MAC:
mp = (struct fip_mac_desc *)desc;
QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_LL2,
- "fd_mac=%pM.\n", __func__, mp->fd_mac);
+ "fd_mac=%pM\n", mp->fd_mac);
ether_addr_copy(cvl_mac, mp->fd_mac);
break;
case FIP_DT_NAME:
diff --git a/drivers/scsi/qedf/qedf_io.c b/drivers/scsi/qedf/qedf_io.c
index ee0dcf9d3aba..46debe5034af 100644
--- a/drivers/scsi/qedf/qedf_io.c
+++ b/drivers/scsi/qedf/qedf_io.c
@@ -1342,7 +1342,7 @@ void qedf_scsi_completion(struct qedf_ctx *qedf, struct
fcoe_cqe *cqe,
} else {
refcount = kref_read(&io_req->refcount);
QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_IO,
- "%d:0:%d:%d xid=0x%0x op=0x%02x "
+ "%d:0:%d:%lld xid=0x%0x op=0x%02x "
"lba=%02x%02x%02x%02x cdb_status=%d "
"fcp_resid=0x%x refcount=%d.\n",
qedf->lport->host->host_no, sc_cmd->device->id,
@@ -1426,7 +1426,7 @@ void qedf_scsi_done(struct qedf_ctx *qedf, struct
qedf_ioreq *io_req,
sc_cmd->result = result << 16;
refcount = kref_read(&io_req->refcount);
- QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_IO, "%d:0:%d:%d: Completing "
+ QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_IO, "%d:0:%d:%lld: Completing "
"sc_cmd=%p result=0x%08x op=0x%02x lba=0x%02x%02x%02x%02x, "
"allowed=%d retries=%d refcount=%d.\n",
qedf->lport->host->host_no, sc_cmd->device->id,
--
2.10.0.rc2.1.g053435c
[PATCH] qla2xxx: Fix ql_dump_buffer
Recent printk changes for KERN_CONT cause this logging to be
defectively emitted on multiple lines. Fix it.
Also reduces object size a trivial amount.
$ size drivers/scsi/qla2xxx/qla_dbg.o*
textdata bss dec hex filename
39125 0 0 3912598d5 drivers/scsi/qla2xxx/qla_dbg.o.new
39164 0 0 3916498fc drivers/scsi/qla2xxx/qla_dbg.o.old
Signed-off-by: Joe Perches
---
drivers/scsi/qla2xxx/qla_dbg.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index 21d9fb7fc887..51b4179469d1 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -2707,13 +2707,9 @@ ql_dump_buffer(uint32_t level, scsi_qla_host_t *vha,
int32_t id,
"%-+5d 0 1 2 3 4 5 6 7 8 9 A B C D E F\n", size);
ql_dbg(level, vha, id,
"- ---\n");
- for (cnt = 0; cnt < size; cnt++, buf++) {
- if (cnt % 16 == 0)
- ql_dbg(level, vha, id, "%04x:", cnt & ~0xFU);
- printk(" %02x", *buf);
- if (cnt % 16 == 15)
- printk("\n");
+ for (cnt = 0; cnt < size; cnt += 16) {
+ ql_dbg(level, vha, id, "%04x: ", cnt);
+ print_hex_dump(KERN_CONT, "", DUMP_PREFIX_NONE, 16, 1,
+ buf + cnt, min(16U, size - cnt), false);
}
- if (cnt % 16 != 0)
- printk("\n");
}
--
2.10.0.rc2.1.g053435c
Re: [PATCH v4 00/19] Replace PCI pool by DMA pool API
On Wed, 2017-03-01 at 16:55 +0100, Romain Perier wrote: > support to warn about this old API in checkpath.pl checkpatch This part isn't true anymore, but it seems sensible enough, thanks.
Re: [PATCH v3 20/20] checkpatch: warn for use of old PCI pool API
On Mon, 2017-02-27 at 13:52 +0100, Romain Perier wrote: > > I also wonder if you've in fact converted all of the > > pci_pool struct and function uses why a new checkpatch > > test is needed at all. > > That's just to avoid futures mistakes/uses. When all instances and macro definitions are removed the check is pointless as any newly submitted patch will not compile.
Re: [PATCH v3 20/20] checkpatch: warn for use of old PCI pool API
On Mon, 2017-02-27 at 13:26 +0100, Romain Perier wrote:
> Hello,
>
>
> Le 27/02/2017 à 12:22, Peter Senna Tschudin a écrit :
> > On Sun, Feb 26, 2017 at 08:24:25PM +0100, Romain Perier wrote:
> > > pci_pool_*() functions should be replaced by the corresponding functions
> > > in the DMA pool API. This adds support to check for use of these pci
> > > functions and display a warning when it is the case.
> > >
> >
> > I guess Joe Perches did sent some comments for this one, did you address
> > them?
>
> See the changelog of 00/20 (for v2). I have already integrated his
> comments :)
Not quite. You need to add blank lines before and after
the new test you added.
I also wonder if you've in fact converted all of the
pci_pool struct and function uses why a new checkpatch
test is needed at all.
Also, it seems none of these patches have reached lkml.
Are you sending the patch series with MIME/html parts?
> > Reviewed-by: Peter Senna Tschudin
> > > Signed-off-by: Romain Perier
> > > ---
> > > scripts/checkpatch.pl | 9 -
> > > 1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index baa3c7b..f2c775c 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -6064,7 +6064,14 @@ sub process {
> > > WARN("USE_DEVICE_INITCALL",
> > >"please use device_initcall() or more appropriate
> > > function instead of __initcall() (see include/linux/init.h)\n" .
> > > $herecurr);
> > > }
> > > -
> > > +# check for old PCI api pci_pool_*(), use dma_pool_*() instead
> > > + if ($line =~
> > > /\bpci_pool(?:_(?:create|destroy|alloc|zalloc|free)|)\b/) {
> > > + if (WARN("USE_DMA_POOL",
> > > + "please use the dma pool api or more
> > > appropriate function instead of the old pci pool api\n" . $herecurr) &&
> > > + $fix) {
> > > + while ($fixed[$fixlinenr] =~
> > > s/\bpci_pool(_(?:create|destroy|alloc|zalloc|free)|)\b/dma_pool$1/) {}
> > > + }
> > > + }
> > > # check for various structs that are normally const (ops, kgdb,
> > > device_tree)
> > > if ($line !~ /\bconst\b/ &&
> > > $line =~ /\bstruct\s+($const_structs)\b/) {
> > > --
> > > 2.9.3
Re: [PATCH v3 20/20] checkpatch: warn for use of old PCI pool API
On Mon, 2017-02-27 at 12:22 +0100, Peter Senna Tschudin wrote:
> On Sun, Feb 26, 2017 at 08:24:25PM +0100, Romain Perier wrote:
> > pci_pool_*() functions should be replaced by the corresponding functions
> > in the DMA pool API. This adds support to check for use of these pci
> > functions and display a warning when it is the case.
> >
>
> I guess Joe Perches did sent some comments for this one, did you address
> them?
> Reviewed-by: Peter Senna Tschudin
> > Signed-off-by: Romain Perier
> > ---
> > scripts/checkpatch.pl | 9 -
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index baa3c7b..f2c775c 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -6064,7 +6064,14 @@ sub process {
> > WARN("USE_DEVICE_INITCALL",
> > "please use device_initcall() or more appropriate
> > function instead of __initcall() (see include/linux/init.h)\n" . $herecurr);
> > }
> > -
> > +# check for old PCI api pci_pool_*(), use dma_pool_*() instead
> > + if ($line =~
> > /\bpci_pool(?:_(?:create|destroy|alloc|zalloc|free)|)\b/) {
> > + if (WARN("USE_DMA_POOL",
> > +"please use the dma pool api or more
> > appropriate function instead of the old pci pool api\n" . $herecurr) &&
> > + $fix) {
> > + while ($fixed[$fixlinenr] =~
> > s/\bpci_pool(_(?:create|destroy|alloc|zalloc|free)|)\b/dma_pool$1/) {}
> > + }
> > + }
> > # check for various structs that are normally const (ops, kgdb,
> > device_tree)
> > if ($line !~ /\bconst\b/ &&
> > $line =~ /\bstruct\s+($const_structs)\b/) {
> >
This is nearly identical to the suggestion that I
sent but this is slightly misformatted as it does
not have a leading nor a trailing blank line to
separate the test blocks.
Also, I think none of the patches have reached lkml.
Romain, are you using git-send-email to send these
patches? Perhaps the patches you send also contain
html which are rejected by the mailing list.
Re: [PATCH 00/35] treewide trivial patches converting pr_warning to pr_warn
On Thu, 2017-02-23 at 17:41 +, Emil Velikov wrote: > On 23 February 2017 at 17:18, Joe Perches wrote: > > On Thu, 2017-02-23 at 09:28 -0600, Rob Herring wrote: > > > On Fri, Feb 17, 2017 at 1:11 AM, Joe Perches wrote: > > > > There are ~4300 uses of pr_warn and ~250 uses of the older > > > > pr_warning in the kernel source tree. > > > > > > > > Make the use of pr_warn consistent across all kernel files. > > > > > > > > This excludes all files in tools/ as there is a separate > > > > define pr_warning for that directory tree and pr_warn is > > > > not used in tools/. > > > > > > > > Done with 'sed s/\bpr_warning\b/pr_warn/' and some emacsing. > > > > [] > > > Where's the removal of pr_warning so we don't have more sneak in? > > > > After all of these actually get applied, > > and maybe a cycle or two later, one would > > get sent. > > > > By which point you'll get a few reincarnation of it. So you'll have to > do the same exercise again :-( Maybe to one or two files. Not a big deal. > I guess the question is - are you expecting to get the series merged > all together/via one tree ? No. The only person that could do that effectively is Linus. > If not, your plan is perfectly reasonable.
Re: [PATCH 00/35] treewide trivial patches converting pr_warning to pr_warn
On Thu, 2017-02-23 at 09:28 -0600, Rob Herring wrote: > On Fri, Feb 17, 2017 at 1:11 AM, Joe Perches wrote: > > There are ~4300 uses of pr_warn and ~250 uses of the older > > pr_warning in the kernel source tree. > > > > Make the use of pr_warn consistent across all kernel files. > > > > This excludes all files in tools/ as there is a separate > > define pr_warning for that directory tree and pr_warn is > > not used in tools/. > > > > Done with 'sed s/\bpr_warning\b/pr_warn/' and some emacsing. [] > Where's the removal of pr_warning so we don't have more sneak in? After all of these actually get applied, and maybe a cycle or two later, one would get sent.
[PATCH 00/35] treewide trivial patches converting pr_warning to pr_warn
There are ~4300 uses of pr_warn and ~250 uses of the older pr_warning in the kernel source tree. Make the use of pr_warn consistent across all kernel files. This excludes all files in tools/ as there is a separate define pr_warning for that directory tree and pr_warn is not used in tools/. Done with 'sed s/\bpr_warning\b/pr_warn/' and some emacsing. Miscellanea: o Coalesce formats and realign arguments Some files not compiled - no cross-compilers Joe Perches (35): alpha: Convert remaining uses of pr_warning to pr_warn ARM: ep93xx: Convert remaining uses of pr_warning to pr_warn arm64: Convert remaining uses of pr_warning to pr_warn arch/blackfin: Convert remaining uses of pr_warning to pr_warn ia64: Convert remaining use of pr_warning to pr_warn powerpc: Convert remaining uses of pr_warning to pr_warn sh: Convert remaining uses of pr_warning to pr_warn sparc: Convert remaining use of pr_warning to pr_warn x86: Convert remaining uses of pr_warning to pr_warn drivers/acpi: Convert remaining uses of pr_warning to pr_warn block/drbd: Convert remaining uses of pr_warning to pr_warn gdrom: Convert remaining uses of pr_warning to pr_warn drivers/char: Convert remaining use of pr_warning to pr_warn clocksource: Convert remaining use of pr_warning to pr_warn drivers/crypto: Convert remaining uses of pr_warning to pr_warn fmc: Convert remaining use of pr_warning to pr_warn drivers/gpu: Convert remaining uses of pr_warning to pr_warn drivers/ide: Convert remaining uses of pr_warning to pr_warn drivers/input: Convert remaining uses of pr_warning to pr_warn drivers/isdn: Convert remaining uses of pr_warning to pr_warn drivers/macintosh: Convert remaining uses of pr_warning to pr_warn drivers/media: Convert remaining use of pr_warning to pr_warn drivers/mfd: Convert remaining uses of pr_warning to pr_warn drivers/mtd: Convert remaining uses of pr_warning to pr_warn drivers/of: Convert remaining uses of pr_warning to pr_warn drivers/oprofile: Convert remaining uses of pr_warning to pr_warn drivers/platform: Convert remaining uses of pr_warning to pr_warn drivers/rapidio: Convert remaining use of pr_warning to pr_warn drivers/scsi: Convert remaining use of pr_warning to pr_warn drivers/sh: Convert remaining use of pr_warning to pr_warn drivers/tty: Convert remaining uses of pr_warning to pr_warn drivers/video: Convert remaining uses of pr_warning to pr_warn kernel/trace: Convert remaining uses of pr_warning to pr_warn lib: Convert remaining uses of pr_warning to pr_warn sound/soc: Convert remaining uses of pr_warning to pr_warn arch/alpha/kernel/perf_event.c | 4 +- arch/arm/mach-ep93xx/core.c| 4 +- arch/arm64/include/asm/syscall.h | 8 ++-- arch/arm64/kernel/hw_breakpoint.c | 8 ++-- arch/arm64/kernel/smp.c| 4 +- arch/blackfin/kernel/nmi.c | 2 +- arch/blackfin/kernel/ptrace.c | 2 +- arch/blackfin/mach-bf533/boards/stamp.c| 2 +- arch/blackfin/mach-bf537/boards/cm_bf537e.c| 2 +- arch/blackfin/mach-bf537/boards/cm_bf537u.c| 2 +- arch/blackfin/mach-bf537/boards/stamp.c| 2 +- arch/blackfin/mach-bf537/boards/tcm_bf537.c| 2 +- arch/blackfin/mach-bf561/boards/cm_bf561.c | 2 +- arch/blackfin/mach-bf561/boards/ezkit.c| 2 +- arch/blackfin/mm/isram-driver.c| 4 +- arch/ia64/kernel/setup.c | 6 +-- arch/powerpc/kernel/pci-common.c | 4 +- arch/powerpc/mm/init_64.c | 5 +-- arch/powerpc/mm/mem.c | 3 +- arch/powerpc/platforms/512x/mpc512x_shared.c | 4 +- arch/powerpc/platforms/85xx/socrates_fpga_pic.c| 7 ++-- arch/powerpc/platforms/86xx/mpc86xx_hpcn.c | 2 +- arch/powerpc/platforms/pasemi/dma_lib.c| 4 +- arch/powerpc/platforms/powernv/opal.c | 8 ++-- arch/powerpc/platforms/powernv/pci-ioda.c | 10 ++--- arch/powerpc/platforms/ps3/device-init.c | 14 +++ arch/powerpc/platforms/ps3/mm.c| 4 +- arch/powerpc/platforms/ps3/os-area.c | 2 +- arch/powerpc/platforms/pseries/iommu.c | 8 ++-- arch/powerpc/platforms/pseries/setup.c | 4 +- arch/powerpc/sysdev/fsl_pci.c | 9 ++--- arch/powerpc/sysdev/mpic.c | 10 ++--- arch/powerpc/sysdev/xics/icp-native.c | 10 ++--- arch/powerpc/sysdev/xics/ics-opal.c| 4 +- arch/powerpc/sysdev/xics/ics-rtas.c| 4 +- arch/powerpc/sysdev/xics/xics-common.c | 8 ++-- arch/sh/boards/mach-sdk7786/nmi.c | 2 +- arch/sh/drivers/pci/fixups-sdk7786.c | 2 +- arch/sh/kernel/io
[PATCH 29/35] drivers/scsi: Convert remaining use of pr_warning to pr_warn
To enable eventual removal of pr_warning
This makes pr_warn use consistent for drivers/scsi
Prior to this patch, there was 1 use of pr_warning and
96 uses of pr_warn in drivers/scsi
Signed-off-by: Joe Perches
---
drivers/scsi/a3000.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/a3000.c b/drivers/scsi/a3000.c
index e6375b4de79e..2be0f577abbf 100644
--- a/drivers/scsi/a3000.c
+++ b/drivers/scsi/a3000.c
@@ -38,7 +38,7 @@ static irqreturn_t a3000_intr(int irq, void *data)
spin_unlock_irqrestore(instance->host_lock, flags);
return IRQ_HANDLED;
}
- pr_warning("Non-serviced A3000 SCSI-interrupt? ISTR = %02x\n", status);
+ pr_warn("Non-serviced A3000 SCSI-interrupt? ISTR = %02x\n", status);
return IRQ_NONE;
}
--
2.10.0.rc2.1.g053435c
Re: [RFC 19/19] checkpatch: warn for use of old PCI pool API
On Wed, 2017-02-08 at 19:55 +0100, Peter Senna Tschudin wrote:
> On Wed, Feb 08, 2017 at 05:34:57PM +0100, Romain Perier wrote:
> > pci_pool_*() functions should be replaced by the corresponding functions
> > in the DMA pool API. This adds support to check for use of these pci
> > functions and display a warning when it is the case.
>
> Don't know if relevant, but did not catch the struct. Other than that
> works fine.
>
> >
> > Signed-off-by: Romain Perier
> > ---
> > scripts/checkpatch.pl | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 8e96af5..026920e 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -6058,6 +6058,11 @@ sub process {
> > WARN("USE_DEVICE_INITCALL",
> > "please use device_initcall() or more appropriate
> > function instead of __initcall() (see include/linux/init.h)\n" . $herecurr);
> > }
> > +# check for old PCI api pci_pool_*(), use dma_pool_*() instead
> > + if ($line =~ /\bpci_pool_.+\(/) {
> > + WARN("USE_PCI_POOL",
> > +"please use the dma pool api or more appropriate
> > function instead of the old pci pool api\n" . $herecurr);
> > + }
> >
> > # check for various structs that are normally const (ops, kgdb,
> > device_tree)
> > if ($line !~ /\bconst\b/ &&
> > --
> > 2.9.3
> >
Did this patch get to lkml?
Perhaps this would be more complete:
scripts/checkpatch.pl | 9 +
1 file changed, 9 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 8e96af53611c..600f81cc1ec1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6059,6 +6059,15 @@ sub process {
"please use device_initcall() or more appropriate
function instead of __initcall() (see include/linux/init.h)\n" . $herecurr);
}
+# check for old PCI api pci_pool_*(), use dma_pool_*() instead
+ if ($line =~
/\bpci_pool(?:_(?:create|destroy|alloc|zalloc|free)|)\b/) {
+ if (WARN("USE_DMA_POOL",
+ "please use the dma pool api or more
appropriate function instead of the old pci pool api\n" . $herecurr) &&
+ $fix) {
+ while ($fixed[$fixlinenr] =~
s/\bpci_pool(_(?:create|destroy|alloc|zalloc|free)|)\b/dma_pool$1/) {}
+ }
+ }
+
# check for various structs that are normally const (ops, kgdb, device_tree)
if ($line !~ /\bconst\b/ &&
$line =~ /\bstruct\s+($const_structs)\b/) {
Re: [PATCH] net-next: treewide use is_vlan_dev() helper function.
On Fri, 2017-02-03 at 22:26 -0600, Parav Pandit wrote: > This patch makes use of is_vlan_dev() function instead of flag > comparison which is exactly done by is_vlan_dev() helper function. Thanks. btw: after applying this patch, there is one left $ git grep -E -n "&\s*IFF_802_1Q_VLAN\b" -- "*.c" drivers/net/ethernet/chelsio/cxgb4/l2t.c:435: if (neigh->dev->priv_flags & IFF_802_1Q_VLAN)
Re: [PATCH v2 1/4] mpt3sas: Added print to notify cable running at a degraded speed.
On Fri, 2017-01-20 at 09:55 -0800, Joe Perches wrote:
> I believe MPT3SAS_FMT is unnecessary obfuscation and
> it should just be replaced by "%s: " everywhere.
Here's a trivial command that could be used one day:
$ git grep --name-only MPT3SAS_FMT -- "*.c" | \
xargs perl -p -i -e 'local $/; while (<>) { s/\bMPT3SAS_FMT\s*"/"%s: /g;
print; }'
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] mpt3sas: Added print to notify cable running at a degraded speed.
On Fri, 2017-01-20 at 20:12 +0530, Chaitra P B wrote:
> Driver processes the event MPI26_EVENT_ACTIVE_CABLE_DEGRADED
> when a cable is present and is running at a degraded speed
> (below the SAS3 12 Gb/s rate). Prints added
> to inform the user that the cable is not running at
> optimal speed.
[]
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
[]
> @@ -8028,15 +8028,23 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER
> *ioc, u8 msix_index,
> case MPI2_EVENT_ACTIVE_CABLE_EXCEPTION:
> ActiveCableEventData =
> (Mpi26EventDataActiveCableExcept_t *) mpi_reply->EventData;
> - if (ActiveCableEventData->ReasonCode ==
> - MPI26_EVENT_ACTIVE_CABLE_INSUFFICIENT_POWER) {
> - pr_info(MPT3SAS_FMT "Currently an active cable with
> ReceptacleID %d",
> + switch (ActiveCableEventData->ReasonCode) {
> + case MPI26_EVENT_ACTIVE_CABLE_INSUFFICIENT_POWER:
> + pr_info(MPT3SAS_FMT "Currently an active cable with
> ReceptacleID %d\n",
> ioc->name, ActiveCableEventData->ReceptacleID);
> - pr_info("cannot be powered and devices connected to
> this active cable");
> - pr_info("will not be seen. This active cable");
> - pr_info("requires %d mW of power",
> + pr_info(" cannot be powered and devices connected
> to\n");
> + pr_info(" this active cable will not be seen. This\n");
> + pr_info(" cable requires %d mW of power\n",
Can you please use more intelligible logging
where sentences are not broken across multiple
lines of output? Something like:
pr_notice(MPT3SAS_FMT "Receptacle ID %d: This active
cable requires %d mW of power\n",
ioc->name, ActiveCableEventData->ReceptacleID,
ActiveCableEventData->ActiveCablePowerRequirement);
pr_notice(MPT3SAS_FMT "Receptacle ID %d: Devices
connected to this active cable will not be seen\n",
ioc->name,
ActiveCableEventData->ReceptacleID);
I believe MPT3SAS_FMT is unnecessary obfuscation and
it should just be replaced by "%s: " everywhere.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 12/12] scsi: ufs: Improve fatal error logs
On Mon, 2016-12-12 at 16:56 -0800, Subhash Jadavani wrote:
> Errors such as UIC error, illegal OCS values, and others may require
> more information for debugging. Such information could be hibern8 events,
> events sequences, recoverable errors, error history, and more.
[]
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
[]
> @@ -346,6 +346,37 @@ static inline void ufshcd_cond_add_cmd_trace(struct
> ufs_hba *hba,
[]
> +static void ufshcd_print_uic_err_hist(struct ufs_hba *hba,
> + struct ufs_uic_err_reg_hist *err_hist, char *err_name)
> +{
> + int i;
> +
> + for (i = 0; i < UIC_ERR_REG_HIST_LENGTH; i++) {
> + int p = (i + err_hist->pos - 1) % UIC_ERR_REG_HIST_LENGTH;
> +
> + if (err_hist->reg[p] == 0)
> + continue;
> + dev_err(hba->dev, "%s[%d] = 0x%x at %lld us", err_name, i,
> + err_hist->reg[p], ktime_to_us(err_hist->tstamp[p]));
Please consistently use a terminating \n
> + }
> +}
> +
> static void ufshcd_print_host_regs(struct ufs_hba *hba)
> {
> /*
> @@ -362,6 +393,21 @@ static void ufshcd_print_host_regs(struct ufs_hba *hba)
> dev_err(hba->dev,
> "hba->outstanding_reqs = 0x%x, hba->outstanding_tasks = 0x%x",
> (u32)hba->outstanding_reqs, (u32)hba->outstanding_tasks);
> + dev_err(hba->dev,
> + "last_hibern8_exit_tstamp at %lld us, hibern8_exit_cnt = %d",
etc...
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi/qla2xxx: label endian-ness for many fields
On Fri, 2016-12-09 at 22:45 +0200, Michael S. Tsirkin wrote:
> This adds endian-ness labels for lots of qla structs.
> Doing this cuts down number of sparse warnings from ~1700 to ~1400.
> Will help find and resolve some of real issues down the road.
>
> Signed-off-by: Michael S. Tsirkin
>
> ---
>
> Compile-tested only.
>
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index 73b12e4..a4d3071 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -1159,28 +1159,28 @@ typedef struct {
>*/
> uint8_t firmware_options[2];
>
> - uint16_t frame_payload_size;
> - uint16_t max_iocb_allocation;
> - uint16_t execution_throttle;
> + __le16 frame_payload_size;
> + __le16 max_iocb_allocation;
> + __le16 execution_throttle;
Shouldn't all these _not_ have the leading __?
Perhaps the uint8_t uses should be converted to u8 as well.
[etc...]
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sd: assign appropriate log level
On Mon, 2016-10-17 at 09:51 -0700, David Singleton wrote:
> From: Shikhar Dogra
>
> Reduce chatter on console for usb hotplug.
> KERN_ERR is too high severity for these messages, moving them
> to KERN_WARNING
Perhaps KERN_NOTICE is more appropriate.
That's the level for most of these sd_first_printk already.
> USB devices never have a Caching Mode page, it doesn't make
> sense to make it an error when you have tons of USB devices where
> the print is useless, and not an error.
>
> For second message, the condition is not an error. The existing
> workaround of assuming a write through cache doesn't limit
> functionality in any way.
>
> Cc: xe-ker...@external.cisco.com
> Signed-off-by: Shikhar Dogra
> Signed-off-by: David Singleton
> ---
> drivers/scsi/sd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 51e5629..ab7bfe3 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2540,7 +2540,7 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned
> char *buffer)
> }
> }
>
> - sd_first_printk(KERN_ERR, sdkp, "No Caching mode page found\n");
> + sd_first_printk(KERN_WARNING, sdkp, "No Caching mode page
> found\n");
> goto defaults;
>
> Page_found:
> @@ -2594,7 +2594,7 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned
> char *buffer)
> "Assuming drive cache: write back\n");
> sdkp->WCE = 1;
> } else {s
> - sd_first_printk(KERN_ERR, sdkp,
> + sd_first_printk(KERN_WARNING, sdkp,
> "Assuming drive cache: write through\n");
> sdkp->WCE = 0;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/26] constify local structures
On Sun, 2016-09-11 at 15:05 +0200, Julia Lawall wrote:
> Constify local structures.
Thanks Julia.
A few suggestions & questions:
Perhaps the script should go into scripts/coccinelle/
so that future cases could be caught by the robot
and commit message referenced by the patch instances.
Can you please compile the files modified using the
appropriate defconfig/allyesconfig and show the
movement from data to const by using
$ size .new/old
and include that in the changelogs (maybe next time)?
Is it possible for a rule to trace the instances where
an address of a struct or struct member is taken by
locally defined and declared function call where the
callee does not modify any dereferenced object?
ie:
struct foo {
int bar;
char *baz;
};
struct foo qux[] = {
{ 1, "description 1" },
{ 2, "dewcription 2" },
[ n, "etc" ]...,
};
void message(struct foo *msg)
{
printk("%d %s\n", msg->bar, msg->baz);
}
where some code uses
message(qux[index]);
So could a coccinelle script change:
struct foo qux[] = { to const struct foo quz[] = {
and
void message(struct foo *msg) to void message(const struct foo *msg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] wd719x: Remove last declaration using DEFINE_PCI_DEVICE_TABLE
Convert it to the preferred const struct pci_device_id instead.
Signed-off-by: Joe Perches
---
drivers/scsi/wd719x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c
index e3da1a2..2a9da2e 100644
--- a/drivers/scsi/wd719x.c
+++ b/drivers/scsi/wd719x.c
@@ -962,7 +962,7 @@ static void wd719x_pci_remove(struct pci_dev *pdev)
scsi_host_put(sh);
}
-static DEFINE_PCI_DEVICE_TABLE(wd719x_pci_table) = {
+static const struct pci_device_id wd719x_pci_table[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_WD, 0x3296) },
{}
};
--
2.10.0.rc2.1.gb2aa91d
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Remove last use and references to DEFINE_PCI_DEVICE_TABLE
Joe Perches (2): wd719x: Remove last declaration using DEFINE_PCI_DEVICE_TABLE treewide: Remove references to the now unnecessary DEFINE_PCI_DEVICE_TABLE Documentation/PCI/pci.txt | 1 - drivers/scsi/wd719x.c | 2 +- include/linux/pci.h | 9 - scripts/checkpatch.pl | 9 - scripts/tags.sh | 1 - 5 files changed, 1 insertion(+), 21 deletions(-) -- 2.10.0.rc2.1.gb2aa91d -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] be2iscsi: Use a more current logging style
On Wed, 2016-08-17 at 09:20 +0530, Jitendra Bhivare wrote:
> >
> > -Original Message-
> > From: Joe Perches [mailto:j...@perches.com]
> > Sent: Tuesday, August 16, 2016 3:57 PM
> > To: Jitendra Bhivare; Christophe JAILLET; Jayamohan Kallickal; Ketan
> Mukadam
> >
> > Cc: Bart Van Assche; James E.J. Bottomley; Martin K. Petersen; linux-
> > s...@vger.kernel.org; linux-ker...@vger.kernel.org
> > Subject: Re: [PATCH] be2iscsi: Use a more current logging style
> >
> > On Tue, 2016-08-16 at 11:32 +0530, Jitendra Bhivare wrote:
> > >
> > > Thanks Joe for taking this up. It has been pending for long time from
> > > our side.
> > Thanks, not a problem, it took ~10 minutes.
> >
> > There was a bit of an issue about your reply though.
> >
> > First there was ~50 k of quoted stuff without any content
> >
> > >
> > > [ hundreds and hundreds of quoted lines ]
> > and then this happened:
> >
> > >
> > > >
> > > > diff --git a/drivers/scsi/be2iscsi/be_main.h
> > > b/drivers/scsi/be2iscsi/be_main.h
> > > >
> > > >
> > > > index aa9c682..7cce6e3 100644
> > > > --- a/drivers/scsi/be2iscsi/be_main.h
> > > > +++ b/drivers/scsi/be2iscsi/be_main.h
> > > > @@ -1081,15 +1081,19 @@ struct hwi_context_memory {
> > > > #define BEISCSI_LOG_CONFIG 0x0020 /* CONFIG Code Path */
> > > > #define BEISCSI_LOG_ISCSI 0x0040 /* SCSI/iSCSI Protocol
> related
> >
> > >
> > > Logs */
> > > >
> > > >
> > > >
> > > > -#define __beiscsi_log(phba, level, fmt, ...)
> \
> >
> > >
> > > >
> > > > - shost_printk(level, phba->shost, fmt, ##__VA_ARGS__)
> > > > -
> > > > -#define beiscsi_log(phba, level, mask, prefix, fmt, ...)
> > > > \
> > > > +#define beiscsi_printk(level, phba, mask, fmt, ...)
> \
> >
> > >
> > > >
> > > > do {
> > \
> > >
> > > >
> > > > - uint32_t log_value = phba->attr_log_enable;
> > > > \
> > > > - if (((mask) & log_value) || (level[1] <= '3'))
> > > > \
> > > > - __beiscsi_log(phba, level, prefix "_%d: " fmt,
> > > > \
> > > > - __LINE__, ##__VA_ARGS__);
> > > > \
> > > > + if ((mask) & (phba)->attr_log_enable)
> > > > \
> > > > + shost_printk(level, phba->shost,
> > > > \
> > > [JB] PCI dev_printk would be more useful with SCSI host_no included by
> > > default in the message.
> > This is a good note that seems simple enough, but I almost missed this.
> >
> > Given the reply at the top and the _very_ long uncommented quoted block,
> I just
> >
> > about assumed it was a useless block quote that you didn't bother to
> trim.
> >
> >
> > Please make it easier to find your replies and notes by deleting
> irrelevant quoted
> >
> > stuff.
> >
> > Also, I think I misread the code.
> >
> > The original code is <= '3' i.e.: show all KERN_ERR.
> > That is not correct in the new code.
> >
> > I don't know the code well and don't have a test bed with the hardware.
> >
> > Is it possible for a beiscsi_ message to be called before
> phba->pcidev is
> >
> > set to a valid value in beiscsi_hba_alloc? It appears the code is
> careful to only
> >
> > use dev_ logging calls before probe.
> [JB] KERN_ERR messages need to be logged irrespective of the masks.
> I understand, that in some places, mask is unnecessarily passed.
> I had made sure to call __beiscsi_log in some places.
I did as well.
> Can we please keep it that way? So beiscsi_err calls dev_err directly or
> is replaced with dev_err.
No worries, I'll respin the series after Christophe's
patches are applied.
> It's safe to assume pcidev will be valid for all beiscsi_log calls.
> Will test your change on my setup before ack'ing.
Don't bother until you get another patchset.
I suggest you fix your email client when sending
replies to me and to lkml.
What I received is very difficult to read due to
the odd line wrapping.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] be2iscsi: Logging neatening
On Wed, 2016-08-17 at 01:19 +, Bart Van Assche wrote: > On 08/14/16 10:29, Joe Perches wrote: > > On Sun, 2016-08-14 at 17:09 +, Bart Van Assche wrote: > > > My primary concern is how to enable and disable log messages from user > > > space. [] > > I think you are looking for a system wide equivalent > > for the ethtool/netif_ mechanism. > > > > Nothing like that exists currently. > > > > Some code uses a bitmask/and, other code uses a > > level/comparison. [] > As far as I can see all that the ethtool msglevel API implements is a > mechanism to query and set the log level from user space. What various > SCSI drivers implement is not a log level but a log mask mechanism. How > about the following approach to associate a name with each bit in a log > mask, to export these names to user space and to make it possible to > enable/disable messages per log category: > * Introduce a variant of pr_debug() that allows to specify a textual > representation of the log category (a short string without spaces). > * Make the log category names available in > /sys/kernel/debug/dynamic_debug/... > * Today dynamic debug allows to enable/disable log messages by > specifying the source file name, function name, line number, module > name and/or format string. My proposal is to make it also possible to > enable/disable log messages based on the log category name. Many of these logging mechanisms are not just debug facilities. Perhaps a dynamic_debug control would be inappropriate. There have also been various custom scsi log level facilities like the blogic_msg for the very old BusLogic blogic_msg. These functions also sometimes write into some device-specific buffer. Perhaps the largest problem, if this is to be scsi only rather than system wide, is finding out what and how the various bits in a mask should be used. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] be2iscsi: Use a more current logging style
On Tue, 2016-08-16 at 11:32 +0530, Jitendra Bhivare wrote:
> Thanks Joe for taking this up. It has been pending for long time from our
> side.
Thanks, not a problem, it took ~10 minutes.
There was a bit of an issue about your reply though.
First there was ~50 k of quoted stuff without any content
> [ hundreds and hundreds of quoted lines ]
and then this happened:
> > diff --git a/drivers/scsi/be2iscsi/be_main.h
> b/drivers/scsi/be2iscsi/be_main.h
> >
> > index aa9c682..7cce6e3 100644
> > --- a/drivers/scsi/be2iscsi/be_main.h
> > +++ b/drivers/scsi/be2iscsi/be_main.h
> > @@ -1081,15 +1081,19 @@ struct hwi_context_memory {
> > #define BEISCSI_LOG_CONFIG 0x0020 /* CONFIG Code Path */
> > #define BEISCSI_LOG_ISCSI 0x0040 /* SCSI/iSCSI Protocol related
> Logs */
> >
> >
> > -#define __beiscsi_log(phba, level, fmt, ...)
> > \
> > - shost_printk(level, phba->shost, fmt, ##__VA_ARGS__)
> > -
> > -#define beiscsi_log(phba, level, mask, prefix, fmt, ...) \
> > +#define beiscsi_printk(level, phba, mask, fmt, ...)
> > \
> > do {
> > \
> > - uint32_t log_value = phba->attr_log_enable; \
> > - if (((mask) & log_value) || (level[1] <= '3')) \
> > - __beiscsi_log(phba, level, prefix "_%d: " fmt, \
> > - __LINE__, ##__VA_ARGS__); \
> > + if ((mask) & (phba)->attr_log_enable) \
> > + shost_printk(level, phba->shost,\
> [JB] PCI dev_printk would be more useful with SCSI host_no included by
> default in the message.
This is a good note that seems simple enough, but I almost
missed this.
Given the reply at the top and the _very_ long uncommented
quoted block, I just about assumed it was a useless block
quote that you didn't bother to trim.
Please make it easier to find your replies and notes by
deleting irrelevant quoted stuff.
Also, I think I misread the code.
The original code is <= '3' i.e.: show all KERN_ERR.
That is not correct in the new code.
I don't know the code well and don't have a test bed with
the hardware.
Is it possible for a beiscsi_ message to be called
before phba->pcidev is set to a valid value in
beiscsi_hba_alloc? It appears the code is careful to
only use dev_ logging calls before probe.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] be2iscsi: Use a more current logging style
shost_printk macros are converted to beiscsi_ for a
more current logging style.
Consolidate the masked tests into a beiscsi_printk macro and
use that to call shost_printk.
Convert the __beiscsi_log macros to use shost_print directly.
Miscellanea:
o Remove the file prefix identifier and use kbasename(__FILE__)
and stringify(__LINE__) to reduce code size in beiscsi_printk
o Realign arguments
Signed-off-by: Joe Perches
---
drivers/scsi/be2iscsi/be_cmds.c | 142
drivers/scsi/be2iscsi/be_iscsi.c | 223 ++--
drivers/scsi/be2iscsi/be_main.c | 733 +++
drivers/scsi/be2iscsi/be_main.h | 20 +-
drivers/scsi/be2iscsi/be_mgmt.c | 270 +++---
5 files changed, 668 insertions(+), 720 deletions(-)
diff --git a/drivers/scsi/be2iscsi/be_cmds.c b/drivers/scsi/be2iscsi/be_cmds.c
index a51bc0e..574d2f4 100644
--- a/drivers/scsi/be2iscsi/be_cmds.c
+++ b/drivers/scsi/be2iscsi/be_cmds.c
@@ -95,8 +95,8 @@ int be_chk_reset_complete(struct beiscsi_hba *phba)
}
if ((status & 0x8000) || (!num_loop)) {
- beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
- "BC", "Failed in be_chk_reset_complete status =
0x%x\n",
+ beiscsi_err(phba, BEISCSI_LOG_INIT,
+ "Failed in be_chk_reset_complete status = 0x%x\n",
status);
return -EIO;
}
@@ -135,9 +135,9 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba *phba,
spin_lock_bh(&phba->ctrl.mcc_lock);
if (mccq->used == mccq->len) {
- beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
+ beiscsi_err(phba, BEISCSI_LOG_INIT |
BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
- "BC", "MCC queue full: WRB used %u tag avail %u\n",
+ "MCC queue full: WRB used %u tag avail %u\n",
mccq->used, phba->ctrl.mcc_tag_available);
goto alloc_failed;
}
@@ -147,9 +147,9 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba *phba,
tag = phba->ctrl.mcc_tag[phba->ctrl.mcc_alloc_index];
if (!tag) {
- beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
+ beiscsi_err(phba, BEISCSI_LOG_INIT |
BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
- "BC", "MCC tag 0 allocated: tag avail %u alloc
index %u\n",
+ "MCC tag 0 allocated: tag avail %u alloc index
%u\n",
phba->ctrl.mcc_tag_available,
phba->ctrl.mcc_alloc_index);
goto alloc_failed;
@@ -263,10 +263,10 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
set_bit(MCC_TAG_STATE_TIMEOUT,
&phba->ctrl.ptag_state[tag].tag_state);
- beiscsi_log(phba, KERN_ERR,
+ beiscsi_err(phba,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
- "BC", "MBX Cmd Completion timed out\n");
+ "MBX Cmd Completion timed out\n");
return -EBUSY;
}
@@ -289,22 +289,22 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
}
if (status || addl_status) {
- beiscsi_log(phba, KERN_WARNING,
- BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
- BEISCSI_LOG_CONFIG,
- "BC", "MBX Cmd Failed for Subsys : %d Opcode : %d
with Status : %d and Extd_Status : %d\n",
- mbx_hdr->subsystem,
- mbx_hdr->opcode,
- status, addl_status);
+ beiscsi_warn(phba,
+BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
+BEISCSI_LOG_CONFIG,
+"MBX Cmd Failed for Subsys : %d Opcode : %d with
Status : %d and Extd_Status : %d\n",
+mbx_hdr->subsystem,
+mbx_hdr->opcode,
+status, addl_status);
rc = -EIO;
if (status == MCC_STATUS_INSUFFICIENT_BUFFER) {
mbx_resp_hdr = (struct be_cmd_resp_hdr *) mbx_hdr;
- beiscsi_log(phba, KERN_WARNING,
- BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
- BEISCSI_LOG_CONFIG,
- "BC", "Insufficient Buffer Error Resp_Len :
%d Actual_Resp_Len : %d\n",
-
Re: [PATCH 0/2] be2iscsi: Logging neatening
On Sun, 2016-08-14 at 17:09 +, Bart Van Assche wrote: > My primary concern is how to enable and disable log messages from user > space. Many drivers define their own logging macros and export a bitmask > that allows to enable and disable logging messages per category. These > bitmask control mechanisms are annoying because figuring out what bit > controls which message category requires a search through the driver > source code. I'd like to see all these custom logging macros disappear > and being replaced by a single mechanism. The "dynamic debug" mechanism > e.g. is in my opinion much easier to use than the different custom > logging mechanisms. Dynamic debug doesn't have a bitmask function and still requires looking through the code for lines and format strings. I think you are looking for a system wide equivalent for the ethtool/netif_ mechanism. Nothing like that exists currently. Some code uses a bitmask/and, other code uses a level/comparison. Care to propose something? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] be2iscsi: Logging neatening
On Sun, 2016-08-14 at 14:34 +, Bart Van Assche wrote:
> On 08/13/16 13:42, Joe Perches wrote:
> > Joe Perches (2):
> > be2iscsi: Coalesce split strings and formats
> > be2iscsi: Use a standard logging style
> Hello Joe,
Hello Bart.
> As one can see in be_main.h the "level" argument of macro beiscsi_log()
> is ignored for log levels KERN_EMERG, KERN_ALERT, KERN_CRIT and
> KERN_ERR. So for these log levels beiscsi_log() is a synonym of
> shost_printk(). Have you considered to replace beiscsi_log() with
> shost_printk() for these log levels and additionally to change
> beiscsi_log() for the other log levels into pr_debug()? pr_debug()
> statements namely already can be enabled and disabled at runtime. If the
> BEISCSI_LOG_* log category would be embedded in the log text that would
> allow to eliminate the phba->attr_log_enable structure member.
> Additionally, pr_debug() has a facility for displaying the source file
> name and the line number. That would allow to leave out __LINE__ from
> be2iscsi log statements. I don't think it is useful to have that line
> number in non-debug be2iscsi log statements.
My main consideration for submitting a patch at all
was removing the apparent format/argument mismatches.
As far as I can grep, only KERN_ERR, KERN_WARNING and
KERN_INFO are actually used by be2iscsi today.
I agree with the removal of __LINE__ from the macros
as its utility is generally pretty low.
Besides, using stringify(__LINE__) is almost always
smaller object code than a format with "%d", __LINE__.
Prefixes like "BC" and "BS" are __FILE__ equivalents,
and could be removed as well with something like
"%s, kbasename(__FILE__)" used if _really_ desired.
I have no issue with defining and using beiscsi_
equivalents to shost_printks.
I think the test inside beiscsi_log is better removed
with multiple specific beiscsi_ calls used.
I don't know why any KERN_ERR should ever be masked,
but perhaps something like:
#define beiscsi_printk(level, phba, mask, fmt, ...) \
do {\
if ((mask) & (phba)->attr_log_enable) \
shost_printk(level, phba->shost, fmt, ##__VA_ARGS__); \
} while (0)
#define beiscsi_err(phba, mask, fmt, ...) \
beiscsi_printk(KERN_ERR, phba, mask, fmt, ##__VA_ARGS__)
#define beiscsi_warn(phba, mask, fmt, ...) \
beiscsi_printk(KERN_WARNING, phba, mask, fmt, ##__VA_ARGS__)
#define beiscsi_info(phba, mask, fmt, ...) \
beiscsi_printk(KERN_INFO, phba, mask, fmt, ##__VA_ARGS__)
with a sed of the .c files:
$ sed -i 's/beiscsi_log(phba, KERN_ERR/beiscsi_err(phba/g'
drivers/scsi/be2iscsi/*.c
$ sed -i 's/beiscsi_log(phba, KERN_WARNING/beiscsi_warn(phba/g'
drivers/scsi/be2iscsi/*.c
$ sed -i 's/beiscsi_log(phba, KERN_INFO/beiscsi_info(phba/g'
drivers/scsi/be2iscsi/*.c
with argument realignment of those lines.
All of these are of course up to the actual maintainers of be2iscsi.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] be2iscsi: Use a standard logging style
Neaten all the beiscsi_log uses.
Remove the leading 'B_%d" prefixes and make the
format and arguments match without an implied __LINE__.
Signed-off-by: Joe Perches
---
drivers/scsi/be2iscsi/be_cmds.c | 54 +++
drivers/scsi/be2iscsi/be_iscsi.c | 100 ++--
drivers/scsi/be2iscsi/be_main.c | 332 +++
drivers/scsi/be2iscsi/be_main.h | 19 +--
drivers/scsi/be2iscsi/be_mgmt.c | 98 ++--
5 files changed, 300 insertions(+), 303 deletions(-)
diff --git a/drivers/scsi/be2iscsi/be_cmds.c b/drivers/scsi/be2iscsi/be_cmds.c
index cefa342..a51bc0e 100644
--- a/drivers/scsi/be2iscsi/be_cmds.c
+++ b/drivers/scsi/be2iscsi/be_cmds.c
@@ -96,7 +96,7 @@ int be_chk_reset_complete(struct beiscsi_hba *phba)
if ((status & 0x8000) || (!num_loop)) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
- "BC_%d : Failed in be_chk_reset_complete status =
0x%x\n",
+ "BC", "Failed in be_chk_reset_complete status =
0x%x\n",
status);
return -EIO;
}
@@ -137,7 +137,7 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba *phba,
if (mccq->used == mccq->len) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
- "BC_%d : MCC queue full: WRB used %u tag avail
%u\n",
+ "BC", "MCC queue full: WRB used %u tag avail %u\n",
mccq->used, phba->ctrl.mcc_tag_available);
goto alloc_failed;
}
@@ -149,7 +149,7 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba *phba,
if (!tag) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
- "BC_%d : MCC tag 0 allocated: tag avail %u alloc
index %u\n",
+ "BC", "MCC tag 0 allocated: tag avail %u alloc
index %u\n",
phba->ctrl.mcc_tag_available,
phba->ctrl.mcc_alloc_index);
goto alloc_failed;
@@ -266,7 +266,7 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_ERR,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
- "BC_%d : MBX Cmd Completion timed out\n");
+ "BC", "MBX Cmd Completion timed out\n");
return -EBUSY;
}
@@ -292,7 +292,7 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_WARNING,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
- "BC_%d : MBX Cmd Failed for Subsys : %d Opcode : %d
with Status : %d and Extd_Status : %d\n",
+ "BC", "MBX Cmd Failed for Subsys : %d Opcode : %d
with Status : %d and Extd_Status : %d\n",
mbx_hdr->subsystem,
mbx_hdr->opcode,
status, addl_status);
@@ -302,7 +302,7 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_WARNING,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
- "BC_%d : Insufficient Buffer Error Resp_Len
: %d Actual_Resp_Len : %d\n",
+ "BC", "Insufficient Buffer Error Resp_Len :
%d Actual_Resp_Len : %d\n",
mbx_resp_hdr->response_length,
mbx_resp_hdr->actual_resp_len);
rc = -EAGAIN;
@@ -341,7 +341,7 @@ static int beiscsi_process_mbox_compl(struct be_ctrl_info
*ctrl,
if (!compl->flags) {
beiscsi_log(phba, KERN_ERR,
BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
- "BC_%d : BMBX busy, no completion\n");
+ "BC", "BMBX busy, no completion\n");
return -EBUSY;
}
compl->flags = le32_to_cpu(compl->flags);
@@ -363,7 +363,7 @@ static int beiscsi_process_mbox_compl(struct be_ctrl_info
*ctrl,
return 0;
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
- "BC_%d : error in cmd completion: Subsystem : %d Opcode :
%d status(compl/extd)=%d/%d\n",
+ "BC", "error in cmd completi
[PATCH 1/2] be2iscsi: Coalesce split strings and formats
Split strings are not preferred for ease of grep.
Signed-off-by: Joe Perches
---
drivers/scsi/be2iscsi/be_cmds.c | 15 ++--
drivers/scsi/be2iscsi/be_iscsi.c | 33 +++
drivers/scsi/be2iscsi/be_main.c | 180 ++-
drivers/scsi/be2iscsi/be_mgmt.c | 3 +-
4 files changed, 85 insertions(+), 146 deletions(-)
diff --git a/drivers/scsi/be2iscsi/be_cmds.c b/drivers/scsi/be2iscsi/be_cmds.c
index a55eaee..cefa342 100644
--- a/drivers/scsi/be2iscsi/be_cmds.c
+++ b/drivers/scsi/be2iscsi/be_cmds.c
@@ -96,8 +96,8 @@ int be_chk_reset_complete(struct beiscsi_hba *phba)
if ((status & 0x8000) || (!num_loop)) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
- "BC_%d : Failed in be_chk_reset_complete"
- "status = 0x%x\n", status);
+ "BC_%d : Failed in be_chk_reset_complete status =
0x%x\n",
+ status);
return -EIO;
}
@@ -292,9 +292,7 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_WARNING,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
- "BC_%d : MBX Cmd Failed for "
- "Subsys : %d Opcode : %d with "
- "Status : %d and Extd_Status : %d\n",
+ "BC_%d : MBX Cmd Failed for Subsys : %d Opcode : %d
with Status : %d and Extd_Status : %d\n",
mbx_hdr->subsystem,
mbx_hdr->opcode,
status, addl_status);
@@ -304,8 +302,7 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_WARNING,
BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
BEISCSI_LOG_CONFIG,
- "BC_%d : Insufficient Buffer Error "
- "Resp_Len : %d Actual_Resp_Len : %d\n",
+ "BC_%d : Insufficient Buffer Error Resp_Len
: %d Actual_Resp_Len : %d\n",
mbx_resp_hdr->response_length,
mbx_resp_hdr->actual_resp_len);
rc = -EAGAIN;
@@ -1035,8 +1032,8 @@ int beiscsi_cmd_q_destroy(struct be_ctrl_info *ctrl,
struct be_queue_info *q,
int status;
beiscsi_log(phba, KERN_INFO, BEISCSI_LOG_INIT,
- "BC_%d : In beiscsi_cmd_q_destroy "
- "queue_type : %d\n", queue_type);
+ "BC_%d : In beiscsi_cmd_q_destroy queue_type : %d\n",
+ queue_type);
mutex_lock(&ctrl->mbox_lock);
memset(wrb, 0, sizeof(*wrb));
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c
index 09f89a3..d28ac23 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -70,9 +70,8 @@ struct iscsi_cls_session *beiscsi_session_create(struct
iscsi_endpoint *ep,
if (cmds_max > beiscsi_ep->phba->params.wrbs_per_cxn) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_CONFIG,
- "BS_%d : Cannot handle %d cmds."
- "Max cmds per session supported is %d. Using %d."
- "\n", cmds_max,
+ "BS_%d : Cannot handle %d cmds.Max cmds per session
supported is %d. Using %d.\n",
+ cmds_max,
beiscsi_ep->phba->params.wrbs_per_cxn,
beiscsi_ep->phba->params.wrbs_per_cxn);
@@ -139,8 +138,8 @@ beiscsi_conn_create(struct iscsi_cls_session *cls_session,
u32 cid)
phba = iscsi_host_priv(shost);
beiscsi_log(phba, KERN_INFO, BEISCSI_LOG_CONFIG,
- "BS_%d : In beiscsi_conn_create ,cid"
- "from iscsi layer=%d\n", cid);
+ "BS_%d : In beiscsi_conn_create ,cidfrom iscsi layer=%d\n",
+ cid);
cls_conn = iscsi_conn_setup(cls_session, sizeof(*beiscsi_conn), cid);
if (!cls_conn)
@@ -248,8 +247,7 @@ static int beiscsi_create_ipv4_iface(struct beiscsi_hba
*phba)
0, 0);
if (!phba->ipv4_iface) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_CONFIG,
- "BS_%d : Could not "
- "create default IPv4 address.\n");
+ "BS_%d : Could not create default IPv4 address.\n");
return -ENODEV;
[PATCH 0/2] be2iscsi: Logging neatening
Joe Perches (2): be2iscsi: Coalesce split strings and formats be2iscsi: Use a standard logging style drivers/scsi/be2iscsi/be_cmds.c | 61 +++--- drivers/scsi/be2iscsi/be_iscsi.c | 115 ++- drivers/scsi/be2iscsi/be_main.c | 398 +-- drivers/scsi/be2iscsi/be_main.h | 19 +- drivers/scsi/be2iscsi/be_mgmt.c | 99 +- 5 files changed, 314 insertions(+), 378 deletions(-) -- 2.8.0.rc4.16.g56331f8 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2 v3] be2iscsi: Fix some error messages
On Sat, 2016-08-13 at 09:41 -0700, Joe Perches wrote:
> On Sat, 2016-08-13 at 14:31 +0200, Christophe JAILLET wrote:
> > Le 13/08/2016 à 13:35, Joe Perches a écrit :
> > > > @@ -268,7 +268,7 @@ static int beiscsi_eh_abort(struct scsi_cmnd *sc)
> > > > &nonemb_cmd.dma);
> > > > if (nonemb_cmd.va == NULL) {
> > > > beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_EH,
> > > > - "BM_%d : Failed to allocate memory for"
> > > > + "BM_%d : Failed to allocate memory for "
> > > > "mgmt_invalidate_icds\n");
This is the first time I've looked at the beiscsi_log macro.
It sure is odd and undesirable.
It's _very_ not nice to have a format string take an implied
__LINE__ argument.
It'd be much more intelligible to take the first bit as a
separate string, concatenate it in the macro with "_%d: "
and __LINE__ (if that's really useful, I think it's not)
and emit that as the format.
Something like:
diff --git a/drivers/scsi/be2iscsi/be_main.h b/drivers/scsi/be2iscsi/be_main.h
index 30a4606..3f0fbbf 100644
--- a/drivers/scsi/be2iscsi/be_main.h
+++ b/drivers/scsi/be2iscsi/be_main.h
@@ -1084,11 +1084,12 @@ struct hwi_context_memory {
#define __beiscsi_log(phba, level, fmt, arg...) \
shost_printk(level, phba->shost, fmt, __LINE__, ##arg)
-#define beiscsi_log(phba, level, mask, fmt, arg...) \
-do { \
- uint32_t log_value = phba->attr_log_enable; \
- if (((mask) & log_value) || (level[1] <= '3')) \
- __beiscsi_log(phba, level, fmt, ##arg); \
-} while (0);
+#define beiscsi_log(phba, level, mask, prefix, fmt, ...) \
+do { \
+ uint32_t log_value = phba->attr_log_enable; \
+ if (((mask) & log_value) || (level[1] <= '3')) \
+ __beiscsi_log(phba, level, prefix "_%d: " fmt, \
+ ##__VA_ARGS__); \
+} while (0)
#endif
So these beiscsi_log uses become something like:
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_EH,
"BM", "Failed to allocate memory for
mgmt_invalidate_icds\n");
and the format and its arguments match.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2 v3] be2iscsi: Fix some error messages
On Sat, 2016-08-13 at 14:31 +0200, Christophe JAILLET wrote:
> Le 13/08/2016 à 13:35, Joe Perches a écrit :
> > > @@ -268,7 +268,7 @@ static int beiscsi_eh_abort(struct scsi_cmnd *sc)
> > > &nonemb_cmd.dma);
> > > if (nonemb_cmd.va == NULL) {
> > > beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_EH,
> > > - "BM_%d : Failed to allocate memory for"
> > > + "BM_%d : Failed to allocate memory for "
> > > "mgmt_invalidate_icds\n");
> > doesn't match commit log as no coalescing/concatenation
> > is done.
> >
> > There are many of these.
> >
> I have *only* fixed the one reported by checkpatch and left the others
> unchanged.
>
> My initial proposal was to fix incorrect strings, without modifying too
> much the code. So I decided to do the minimum of changes.
>
> Should I resubmitted with:
> - all strings *in the patch* concatenated?
> - all strings *in the file*" concatenated?
Hello Christophe
You don't _have_ to do anything.
I think the commit message is misleading.
You could submit another patch that does
the equivalent of:
$ ./scripts/checkpatch.pl --types=SPLIT_STRING --fix-inplace
drivers/scsi/be2iscsi/be_main.c
with the appropriate commit message
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2 v3] be2iscsi: Fix some error messages
On Sat, 2016-08-13 at 09:20 +0200, Christophe JAILLET wrote:
> This fixes:
[]
> - concatenate strings on the same line to fix checkpatch warnings
[]
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
[]
> @@ -268,7 +268,7 @@ static int beiscsi_eh_abort(struct scsi_cmnd *sc)
> &nonemb_cmd.dma);
> if (nonemb_cmd.va == NULL) {
> beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_EH,
> - "BM_%d : Failed to allocate memory for"
> + "BM_%d : Failed to allocate memory for "
> "mgmt_invalidate_icds\n");
doesn't match commit log as no coalescing/concatenation
is done.
There are many of these.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/7] lib: string: add functions to case-convert strings
On Tue, 2016-07-05 at 15:36 -0700, Markus Mayer wrote: > On 5 July 2016 at 15:14, Joe Perches wrote: > > On Tue, 2016-07-05 at 13:47 -0700, Markus Mayer wrote: > > > This series introduces a family of generic string case conversion > > > functions. This kind of functionality is needed in several places in > > > the kernel. Right now, everybody seems to be implementing their own > > > copy of this functionality. > > > > > > Based on the discussion of the previous version of this series[1] and > > > the use cases found in the kernel, it does look like having several > > > flavours of case conversion functions is beneficial. The use cases fall > > > into three categories: > > > - copying a string and converting the case while specifying a > > > maximum length to mimic strncpy() > > > - copying a string and converting the case without specifying a > > > length to mimic strcpy() > > > - converting the case of a string in-place (i.e. modifying the > > > string that was passed in) > > > > > > Consequently, I am proposing these new functions: > > > char *strncpytoupper(char *dst, const char *src, size_t len); > > > char *strncpytolower(char *dst, const char *src, size_t len); > > > char *strcpytoupper(char *dst, const char *src); > > > char *strcpytolower(char *dst, const char *src); > > > char *strtoupper(char *s); > > > char *strtolower(char *s); > > I think there isn't much value in anything other > > than strto. > > > > Using str[n]cpy followed by strto is > > pretty obvious and rarely used anyway. > First time around, folks were proposing the "copy" variants when I > submitted just strtolower() by itself[1]. They just asked for source > and destination parameters to strtolower(), but looking at the use > cases that wouldn't have worked so well. Hence it evolved into these 6 > functions. > > Here's a breakdown of how the functions are being used (patches 2-7), > see also [2]: > > Patch 2: strncpytolower() > Patch 3: strtolower() > Patch 4: strncpytolower() and strtolower() > Patch 5: strtolower() > Patch 6: strcpytoupper() > Patch 7: strcpytoupper() > > So it does look like the copy + change case variant is more frequently > used than just strto. Are these functions useful? Not to me, not so much. None of the functions would have the strcpy performance of the arch / asm versions of strcpy and the savings in overall code isn't significant (or measured?). Of course none of the uses are runtime performance important. This patch also adds always compiled functions that aren't used in many .configs. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/7] lib: string: add functions to case-convert strings
On Tue, 2016-07-05 at 13:47 -0700, Markus Mayer wrote: > This series introduces a family of generic string case conversion > functions. This kind of functionality is needed in several places in > the kernel. Right now, everybody seems to be implementing their own > copy of this functionality. > > Based on the discussion of the previous version of this series[1] and > the use cases found in the kernel, it does look like having several > flavours of case conversion functions is beneficial. The use cases fall > into three categories: > - copying a string and converting the case while specifying a > maximum length to mimic strncpy() > - copying a string and converting the case without specifying a > length to mimic strcpy() > - converting the case of a string in-place (i.e. modifying the > string that was passed in) > > Consequently, I am proposing these new functions: > char *strncpytoupper(char *dst, const char *src, size_t len); > char *strncpytolower(char *dst, const char *src, size_t len); > char *strcpytoupper(char *dst, const char *src); > char *strcpytolower(char *dst, const char *src); > char *strtoupper(char *s); > char *strtolower(char *s); I think there isn't much value in anything other than strto. Using str[n]cpy followed by strto is pretty obvious and rarely used anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] scsi: wd7000: print sector number as 64-bit
On Tue, 2016-06-21 at 11:02 +0200, Arnd Bergmann wrote: > Enabling format checking in dprintk() shows that wd7000_biosparam > uses an incorrect format string for sector_t: trivia: > diff --git a/drivers/scsi/wd7000.c b/drivers/scsi/wd7000.c [] > @@ -192,7 +192,7 @@ > #ifdef WD7000_DEBUG > #define dprintk printk > #else > -#define dprintk(format,args...) > +#define dprintk no_printk > #endif It'd be nicer if both defines were the same form #ifdef WD7000_DEBUG #define dprintk printk #else #define dprintk no_printk #endif -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VMW_PVSCSI: Change to update maintainer details (name, email)
On Fri, 2016-06-17 at 17:44 +, Jim Gill wrote:
> On 6/16/16, 8:11 PM, "Julian Calaby" wrote:
>
>
>
> >
> > []
> >
> > >
> > > >
> > > > On Fri, Jun 17, 2016 at 12:33 PM, Joe Perches wrote:
> > > []
> > > The question to me is whether or not Jim Gill is
> > > taking over the maintainership of the entire
> > > VMware PVSCSI driver or just a few files of it.
> > As I see it, he's taking over maintainership of all of it: it's only
> > files are drivers/scsi/vmw_pvscsi.[ch] AFAIK.
> This is correct, I am taking over maintainership of the entire vmw_pvscsi
> driver.
Perhaps a patch like this:
>From e727c6549e3be466ec3c79e919502cb0b9909b03 Mon Sep 17 00:00:00 2001
Message-Id:
From: Joe Perches
Date: Fri, 17 Jun 2016 10:56:49 -0700
Subject: [PATCH] vmw_pvscsi: Move into separate directory, Jim Gill is
MAINTAINER
Separate directories for drivers are generally better.
Miscellanea:
o Update the MAINTAINER entry
o Remove maintainer and FSF addresses from driver files
---
MAINTAINERS| 5 ++---
drivers/scsi/Kconfig | 8 +---
drivers/scsi/Makefile | 2 +-
drivers/scsi/vmware/Kconfig| 7 +++
drivers/scsi/vmware/Makefile | 1 +
drivers/scsi/{ => vmware}/vmw_pvscsi.c | 7 ---
drivers/scsi/{ => vmware}/vmw_pvscsi.h | 7 ---
7 files changed, 12 insertions(+), 25 deletions(-)
create mode 100644 drivers/scsi/vmware/Kconfig
create mode 100644 drivers/scsi/vmware/Makefile
rename drivers/scsi/{ => vmware}/vmw_pvscsi.c (99%)
rename drivers/scsi/{ => vmware}/vmw_pvscsi.h (98%)
diff --git a/MAINTAINERS b/MAINTAINERS
index d174e34..2763582 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12421,12 +12421,11 @@ S:Maintained
F: drivers/net/vmxnet3/
VMware PVSCSI driver
-M: Arvind Kumar
+M: Jim Gill
M: VMware PV-Drivers
L: linux-scsi@vger.kernel.org
S: Maintained
-F: drivers/scsi/vmw_pvscsi.c
-F: drivers/scsi/vmw_pvscsi.h
+F: drivers/scsi/vmware/
VOLTAGE AND CURRENT REGULATOR FRAMEWORK
M: Liam Girdwood
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 1918f54..339c230 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -576,13 +576,7 @@ config SCSI_FLASHPOINT
substantial, so users of MultiMaster Host Adapters may not
wish to include it.
-config VMWARE_PVSCSI
- tristate "VMware PVSCSI driver support"
- depends on PCI && SCSI && X86
- help
- This driver supports VMware's para virtualized SCSI HBA.
- To compile this driver as a module, choose M here: the
- module will be called vmw_pvscsi.
+source "drivers/scsi/vmware/Kconfig"
config XEN_SCSI_FRONTEND
tristate "XEN SCSI frontend driver"
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 862ab4e..6cfefaa 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -141,7 +141,7 @@ obj-$(CONFIG_BE2ISCSI) += libiscsi.o be2iscsi/
obj-$(CONFIG_SCSI_ESAS2R) += esas2r/
obj-$(CONFIG_SCSI_PMCRAID) += pmcraid.o
obj-$(CONFIG_SCSI_VIRTIO) += virtio_scsi.o
-obj-$(CONFIG_VMWARE_PVSCSI)+= vmw_pvscsi.o
+obj-$(CONFIG_VMWARE_PVSCSI)+= vmware/
obj-$(CONFIG_XEN_SCSI_FRONTEND)+= xen-scsifront.o
obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
obj-$(CONFIG_SCSI_WD719X) += wd719x.o
diff --git a/drivers/scsi/vmware/Kconfig b/drivers/scsi/vmware/Kconfig
new file mode 100644
index 000..3c0c53b
--- /dev/null
+++ b/drivers/scsi/vmware/Kconfig
@@ -0,0 +1,7 @@
+config VMWARE_PVSCSI
+ tristate "VMware PVSCSI driver support"
+ depends on PCI && SCSI && X86
+ help
+ This driver supports VMware's para virtualized SCSI HBA.
+ To compile this driver as a module, choose M here: the
+ module will be called vmw_pvscsi.
diff --git a/drivers/scsi/vmware/Makefile b/drivers/scsi/vmware/Makefile
new file mode 100644
index 000..ae8d278
--- /dev/null
+++ b/drivers/scsi/vmware/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VMWARE_PVSCSI)+= vmw_pvscsi.o
diff --git a/drivers/scsi/vmw_pvscsi.c b/drivers/scsi/vmware/vmw_pvscsi.c
similarity index 99%
rename from drivers/scsi/vmw_pvscsi.c
rename to drivers/scsi/vmware/vmw_pvscsi.c
index 6164634..eb1229e 100644
--- a/drivers/scsi/vmw_pvscsi.c
+++ b/drivers/scsi/vmware/vmw_pvscsi.c
@@ -12,13 +12,6 @@
* MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
* NON INFRINGEMENT. See the GNU General Public License for more
* details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
- *
-
Re: [PATCH] VMW_PVSCSI: Change to update maintainer details (name, email)
On Fri, 2016-06-17 at 12:44 +1000, Julian Calaby wrote: > Hi Joe, rehi Julian. > On Fri, Jun 17, 2016 at 12:33 PM, Joe Perches wrote: [] > > get_maintainer.pl also has a rarely used "--file-emails" option to > > scan for what appears to be email addresses in specific files. > > > > $ ./scripts/get_maintainer.pl -f --file-emails drivers/scsi/vmw_pvscsi.c > > Arvind Kumar (maintainer:VMware PVSCSI driver,in > > file) > > VMware PV-Drivers (maintainer:VMware PVSCSI driver) > > "James E.J. Bottomley" (maintainer:SCSI SUBSYSTEM) > > "Martin K. Petersen" (maintainer:SCSI > > SUBSYSTEM) > > linux-scsi@vger.kernel.org (open list:VMware PVSCSI driver) > > linux-ker...@vger.kernel.org (open list) > > > > note the "in file" after Arvind's name > Didn't know this, however my point stands: the maintainer line in the > file is redundant if we find maintainers through MAINTAINERS or the > get_maintainer.pl script. Yes, I'm not suggesting anything else. Jim's name should appear in the MAINTAINERS file somewhere. The question to me is whether or not Jim Gill is taking over the maintainership of the entire VMware PVSCSI driver or just a few files of it. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VMW_PVSCSI: Change to update maintainer details (name, email)
On Fri, 2016-06-17 at 12:18 +1000, Julian Calaby wrote: > ./scripts/get_maintainers.pl -f drivers/scsi/vmw_pvscsi.c just fyi: the script name is not plural $ ./scripts/get_maintainer.pl -f drivers/scsi/vmw_pvscsi.c Arvind Kumar (maintainer:VMware PVSCSI driver) VMware PV-Drivers (maintainer:VMware PVSCSI driver) "James E.J. Bottomley" (maintainer:SCSI SUBSYSTEM) "Martin K. Petersen" (maintainer:SCSI SUBSYSTEM) linux-scsi@vger.kernel.org (open list:VMware PVSCSI driver) linux-ker...@vger.kernel.org (open list) and another fyi: get_maintainer.pl also has a rarely used "--file-emails" option to scan for what appears to be email addresses in specific files. $ ./scripts/get_maintainer.pl -f --file-emails drivers/scsi/vmw_pvscsi.c Arvind Kumar (maintainer:VMware PVSCSI driver,in file) VMware PV-Drivers (maintainer:VMware PVSCSI driver) "James E.J. Bottomley" (maintainer:SCSI SUBSYSTEM) "Martin K. Petersen" (maintainer:SCSI SUBSYSTEM) linux-scsi@vger.kernel.org (open list:VMware PVSCSI driver) linux-ker...@vger.kernel.org (open list) note the "in file" after Arvind's name -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver
On Thu, 2016-06-16 at 16:20 -0500, Bryant G. Ly wrote:
> This driver is a pick up of the old IBM VIO scsi Target Driver
> that was started by Nick and Fujita 2-4 years ago.
> http://comments.gmane.org/gmane.linux.scsi/90119
(style trivia only, nothing important enough to force a respin
but nice to fix one day)
Please use git format-patch -M so file renames are more obvious.
> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> +IBM Power Virtual SCSI Device Target Driver
> +M: Bryant G. Ly
> +M: Michael Cyr
> +L: linux-scsi@vger.kernel.org
> +L: target-de...@vger.kernel.org
> +S: Supportedybe
> +F: drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
Maybe
F: drivers/scsi/ibmscsi_tgt/
> +F: drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h
> +F: drivers/scsi/ibmvscsi_tgt/libsrp.c
> +F: drivers/scsi/ibmvscsi_tgt/libsrp.h
and these become unnecessary
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
[]
> +/**
> + * ibmvscsis_establish_new_q() - Establish new CRQ queue
> + */
If you use kernel-doc comment style,
please describe the function arguments too.
> +static long ibmvscsis_establish_new_q(struct scsi_info *vscsi, uint
> new_state)
> +{
> + long rc = ADAPT_SUCCESS;
> + uint format;
> +
> + vscsi->flags &= PRESERVE_FLAG_FIELDS;
> + vscsi->rsp_q_timer.timer_pops = 0;
> + vscsi->debit = 0;
> + vscsi->credit = 0;
> + rc = vio_enable_interrupts(vscsi->dma_dev);
> + if (rc) {
> + pr_warn("reset_queue: failed to enable interrupts, rc %ld\n",
> + rc);
> + } else {
> + rc = ibmvscsis_check_init_msg(vscsi, &format);
> + if (rc) {
> + dev_err(&vscsi->dev, "reset_queue: check_init_msg
> failed, rc %ld\n",
> + rc);
> + } else if (format == UNUSED_FORMAT &&
> + new_state == WAIT_CONNECTION) {
> + rc = ibmvscsis_send_init_message(vscsi, INIT_MSG);
> + switch (rc) {
> + case H_SUCCESS:
> + case H_DROPPED:
> + case H_CLOSED:
> + rc = ADAPT_SUCCESS;
> + break;
> +
> + case H_PARAMETER:
> + case H_HARDWARE:
> + break;
> +
> + default:
> + vscsi->state = UNDEFINED;
> + rc = H_HARDWARE;
> + break;
> + }
> + }
> + }
> +
> + return rc;
> +}
This sort of code can have indent reduced by doing
rc = vio_enable_interrupts(...)
if (rc) {
pr_warn(...);
return rc;
}
rc = ibmvscsis_check_init_msg(...)
if (rc) {
dev_err(...);
return rc;
}
if (format == UNUSED_FORMAT && new_state == WAIT_CONNECTION) {
rc = ibmvscsis_send_init_message(vscsi, INIT_MSG);
switch (rc) {
etc...
}
}
Why use pr_warn and dev_err in the same function?
Shouldn't dev_ be used whenever a struct device is available?
Also it's nice to use %s, __func__ instead of directly
embedding a function like name into the format string.
A lot of the pr_debug uses seem to be function tracing
and could be eliminated altogether.
> + /* can transition from this state to UNCONFIGURING */
> + case UNDEFINED:
Comment style for switch / case labels could not be indented
but start at the same indent level as the case statement.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver
On Wed, 2016-06-15 at 18:41 -0500, Bryant G. Ly wrote: > The driver provides a virtual SCSI device on IBM Power Servers. [] > MAINTAINERS | 11 + > drivers/scsi/Kconfig | 27 +- > drivers/scsi/Makefile|2 +- > drivers/scsi/ibmvscsi/Makefile |4 + > drivers/scsi/ibmvscsi/ibmvfc.h |2 +- > drivers/scsi/ibmvscsi/ibmvscsi.h |2 +- > drivers/scsi/ibmvscsi/viosrp.h | 225 -- > drivers/scsi/ibmvscsi_tgt/Makefile |4 + > drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 4029 > ++ > drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h | 342 +++ > drivers/scsi/ibmvscsi_tgt/libsrp.c | 427 > drivers/scsi/ibmvscsi_tgt/libsrp.h | 123 + > drivers/scsi/libsrp.c| 447 > include/scsi/libsrp.h| 78 - > include/scsi/viosrp.h| 220 ++ > 15 files changed, 5180 insertions(+), 763 deletions(-) > delete mode 100644 drivers/scsi/ibmvscsi/viosrp.h > create mode 100644 drivers/scsi/ibmvscsi_tgt/Makefile > create mode 100644 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > create mode 100644 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.h > create mode 100644 drivers/scsi/ibmvscsi_tgt/libsrp.c > create mode 100644 drivers/scsi/ibmvscsi_tgt/libsrp.h > delete mode 100644 drivers/scsi/libsrp.c > delete mode 100644 include/scsi/libsrp.h > create mode 100644 include/scsi/viosrp.h > > diff --git a/MAINTAINERS b/MAINTAINERS [] > @@ -5451,6 +5451,17 @@ S: Supported > F: drivers/scsi/ibmvscsi/ibmvscsi* > F: drivers/scsi/ibmvscsi/viosrp.h > > +IBM Power Virtual SCSI Device Target Driver > +M: Bryant G. Ly > +M: Michael Cyr > +L: linux-scsi@vger.kernel.org > +L: target-de...@vger.kernel.org > +S: Supported > +F: drivers/scsi/ibmvscsi/ibmvscsis.c > +F: drivers/scsi/ibmvscsi/ibmvscsis.h > +F: drivers/scsi/ibmvscsi/libsrp.c > +F: drivers/scsi/ibmvscsi/libsrp.h trivia: Please make the MAINTAINER pattern entries match the file locations. This could be done in a separate patch if this doesn't need to be resubmitted for any other reason. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use the correct size to set block max sectors
dOn Thu, 2016-05-26 at 17:08 -0700, Long Li wrote:
> The block sector size should be in unit of 512 bytes, not in bytes.
Thanks. The patch subject should use something like:
[PATCH] sd: Use the correct size to set block max sectors
to show what subsystem is being modified.
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
[]
> @@ -2862,9 +2862,11 @@ static int sd_revalidate_disk(struct gendisk *disk)
> if (sdkp->opt_xfer_blocks &&
> sdkp->opt_xfer_blocks <= dev_max &&
> sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
> - sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE)
> - rw_max = q->limits.io_opt =
> + sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE) {
> + q->limits.io_opt =
> sdkp->opt_xfer_blocks * sdp->sector_size;
> + rw_max = (q->limits.io_opt >> 9);
> + }
> else
> rw_max = BLK_DEF_MAX_SECTORS;
And style trivia: it'd be more kernel style consistent as:
if (...
sdkp->opt_xfer_blocks * sdp->sector_size >= PAGE_SIZE) {
q->limits.io_opt = sdkp->opt_xfer_blocks * sdp->sector_size;
rw_max = q->limits.io_opt >> 9;
} else {
rw_max = BLK_DEF_MAX_SECTORS;
}
ie: no parentheses necessary around the shifted value and
braces around both arms.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] ibmvscsis: clean up functions
On Wed, 2016-05-25 at 09:17 -0500, Bryant G. Ly wrote:
> From: bryantly
Please use your whole name here and for your sign-off like:
From: Bryant G. Ly
Signed-off-by: Bryant G. Ly
> This patch removes forward declarations and re-organizes the
> functions within the driver. This patch also fixes MAINTAINERS
> for ibmvscsis.
trivial note:
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsis.c
> b/drivers/scsi/ibmvscsi/ibmvscsis.c
[]
> static inline long h_copy_rdma(s64 length, u64 sliobn, u64 slioba,
> u64 dliobn, u64 dlioba)
> {
>
Functions like this would be less indented and less
line wrapped for 80 columns if they were written:
if (!se_cmd->residual_count)
return;
[unindented one level...]
etc...
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver
On Tue, 2016-05-24 at 08:52 -0500, Bryant G. Ly wrote:
> From: bgly
>
> This initial commit contains WIP of the IBM VSCSI Target Fabric
> Module. It currently supports read/writes, and I have tested
> the ability to create a file backstore with the driver and install
> RHEL VIA NIM and then boot up the partition via filio backstore
> through the driver.
Only trivial notes:
Maybe try checkpatch with the --strict option and see
if any of the additional messages are important to you.
> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> @@ -5381,6 +5381,16 @@ S: Supported
> F: drivers/scsi/ibmvscsi/ibmvscsi*
> F: drivers/scsi/ibmvscsi/viosrp.h
>
> +IBM Power Virtual SCSI Device Target Driver
> +M: Bryant G. Ly
> +L: linux-scsi@vger.kernel.org
> +L: target-de...@vger.kernel.org
> +S: Supported
> +F: drivers/scsi/ibmvscsi/ibmvscsis.c
> +F: drivers/scsi/ibmvscsi/ibmvscsis.h
> +F: drivers/scsi/libsrp.h
> +F: drivers/scsi/libsrp.c
Please use a tab character consistently after the :
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
[]
> @@ -847,6 +847,20 @@ config SCSI_IBMVSCSI
> To compile this driver as a module, choose M here: the
> module will be called ibmvscsi.
>
> +config SCSI_IBMVSCSIS
> + tristate "IBM Virtual SCSI Server support"
> + depends on PPC_PSERIES && SCSI_SRP && TARGET_CORE
> + help
> + This is the IBM POWER Virtual SCSI Target Server
> +
> + The userspace component needed to initialize the driver and
> + documentation can be found:
here too.
> +
> + https://github.com/powervm/ibmvscsis
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ibmvstgt.
> +
[]
> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
[]
> @@ -127,7 +127,9 @@ obj-$(CONFIG_SCSI_LASI700)+= 53c700.o lasi700.o
> obj-$(CONFIG_SCSI_SNI_53C710)+= 53c700.o sni_53c710.o
> obj-$(CONFIG_SCSI_NSP32) += nsp32.o
> obj-$(CONFIG_SCSI_IPR) += ipr.o
> +obj-$(CONFIG_SCSI_SRP) += libsrp.o
> obj-$(CONFIG_SCSI_IBMVSCSI) += ibmvscsi/
> +obj-$(CONFIG_SCSI_IBMVSCSIS)+= ibmvscsi/
and here
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsis.c
> b/drivers/scsi/ibmvscsi/ibmvscsis.c
[]
> +static int ibmvscsis_probe(struct vio_dev *vdev,
> + const struct vio_device_id *id);
[...]
It might be nice to rearrange the code to avoid these forward
function declarations.
> +static ssize_t ibmvscsis_tpg_enable_store(struct config_item *item,
> + const char *page, size_t count)
> +{
[]
> > + ret = kstrtoul(page, 0, &tmp);
> + if (ret < 0) {
> + pr_err("Unable to extract ibmvscsis_tpg_store_enable\n");
> + return -EINVAL;
> + }
It might be nicer to add:
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
before any #include to output all the logging messages with
a standardized prefix. Then all the other logging with an
embedded prefix can have the embedded prefix removed too.
> +
> + if ((tmp != 0) && (tmp != 1)) {
> + pr_err("Illegal value for ibmvscsis_tpg_store_enable: %lu\n",
> + tmp);
> + return -EINVAL;
> + }
> +
> + if (tmp == 1)
> + tport->enabled = true;
> + else
> + tport->enabled = false;
tport->enabled = tmp;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] megaraid_sas: trivial fix, add missing space in dev_err message
On Mon, 2016-04-25 at 22:58 +0100, Colin King wrote:
> From: Colin Ian King
>
> Add a missing space in dev_err message, missed because the string
> spans a few lines.
This is a dev_notice() not dev_err().
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> b/drivers/scsi/megaraid/megaraid_sas_base.c
[]
> @@ -3345,7 +3345,7 @@ megasas_internal_reset_defer_cmds(struct
> megasas_instance *instance)
> if (!list_empty(&cmd->list)) {
> dev_notice(&instance->pdev->dev, "ERROR while"
> " moving this cmd:%p, %d %p, it was"
> - "discovered on some list?\n",
> + " discovered on some list?\n",
> cmd, cmd->sync_cmd, cmd->scmd);
>
> list_del_init(&cmd->list);
Better would be to coalesce the format, but perhaps
this dev_notice should be dev_err?
dev_notice(&instance->pdev->dev,
"ERROR while moving this cmd:%p, %d
%p, it was discovered on some list?\n",
cmd, cmd->sync_cmd, cmd->scmd);
And the message seems oddly phrased.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cxgbit: fix dma_addr_t printk format
On Sat, 2016-03-05 at 01:34 +0100, Arnd Bergmann wrote:
> On Friday 04 March 2016 16:25:07 Joe Perches wrote:
> > > diff --git a/drivers/target/iscsi/cxgbit/cxgbit_ddp.c
> > > b/drivers/target/iscsi/cxgbit/cxgbit_ddp.c
[]
> > > @@ -179,7 +179,7 @@ cxgbit_dump_sgl(const char *cap, struct scatterlist
> > > *sgl, int nents)
> > > for_each_sg(sgl, sg, nents, i)
> > > pr_info("\t%d/%u, 0x%p: len %u, off %u, pg 0x%p, dma
> > > 0x%llx, %u\n",
> > > i, nents, sg, sg->length, sg->offset, sg_page(sg),
> > > - sg_dma_address(sg), sg_dma_len(sg));
> > > + (u64)sg_dma_address(sg), sg_dma_len(sg));
[]
> > You could create a temporary:
for_each_sg(sgl, sg, nents, i) {
dma_addr_t addr = sg_dma_address(sg);
pr_info("\t%d/%u, 0x%p: len %u, off %u, pg 0x%p, dma %pad,
%u\n",
i, nents, sg, sg->length, sg->offset, sg_page(sg),
&addr, sg_dma_len(sg));
}
Sure, but the cast seemed nicer in this case, the result is the same.
Not quite as 0x%llx isn't always the same width and doesn't
have leading 0's like %pad
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cxgbit: fix dma_addr_t printk format
On Sat, 2016-03-05 at 01:04 +0100, Arnd Bergmann wrote:
> The newly added driver prints a dma_addr_t using the %llx format string,
> but that is wrong on most 32-bit architectures:
>
> drivers/target/iscsi/cxgbit/cxgbit_ddp.c: In function 'cxgbit_dump_sgl':
> drivers/target/iscsi/cxgbit/cxgbit_ddp.c:180:10: error: format '%llx' expects
> argument of type 'long long unsigned int', but argument 8 has type
> 'dma_addr_t {aka unsigned int}' [-Werror=format=]
> pr_info("\t%d/%u, 0x%p: len %u, off %u, pg 0x%p, dma 0x%llx, %u\n",
>
> Unfortunately, we can't use the %pad format string here because
> we are not printing an lvalue, so we have to add a cast to u64, which
> matches the format string on all architectures.
> Signed-off-by: Arnd Bergmann
> Fixes: c49aa56e556d ("cxgbit: add cxgbit_ddp.c")
> ---
> drivers/target/iscsi/cxgbit/cxgbit_ddp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/target/iscsi/cxgbit/cxgbit_ddp.c
> b/drivers/target/iscsi/cxgbit/cxgbit_ddp.c
> index 07e2bc86d0df..d667bc88e21d 100644
> --- a/drivers/target/iscsi/cxgbit/cxgbit_ddp.c
> +++ b/drivers/target/iscsi/cxgbit/cxgbit_ddp.c
> @@ -179,7 +179,7 @@ cxgbit_dump_sgl(const char *cap, struct scatterlist *sgl,
> int nents)
> for_each_sg(sgl, sg, nents, i)
> pr_info("\t%d/%u, 0x%p: len %u, off %u, pg 0x%p, dma 0x%llx,
> %u\n",
> i, nents, sg, sg->length, sg->offset, sg_page(sg),
> - sg_dma_address(sg), sg_dma_len(sg));
> + (u64)sg_dma_address(sg), sg_dma_len(sg));
> }
>
> static int cxgbit_ddp_sgl_check(struct scatterlist *sgl, int nents)
You could create a temporary:
for_each_sg(sgl, sg, nents, i) {
dma_addr_t addr = sg_dma_address(sg);
pr_info("\t%d/%u, 0x%p: len %u, off %u, pg 0x%p, dma %pad,
%u\n",
i, nents, sg, sg->length, sg->offset, sg_page(sg),
&addr, sg_dma_len(sg));
}
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Modified Maintainers list for MPT FUSION DRIVERS.
On Wed, 2016-02-17 at 16:55 +0530, Chaitra P B wrote: > Signed-off-by: Chaitra P B > Suganath prabu Subramani Is this supposed to be a signed-off-by line? A couple questions below. > diff --git a/MAINTAINERS b/MAINTAINERS [] > @@ -6659,10 +6659,9 @@ S: Maintained > F: arch/arm/mach-lpc32xx/ > > LSILOGIC MPT FUSION DRIVERS (FC/SAS/SPI) > -M: Nagalakshmi Nandigama > -M: Praveen Krishnamoorthy > -M: Sreekanth Reddy > -M: Abhijit Mahajan > +M: Sathya Prakash > +M: Chaitra P B > +M: Suganath Prabu Subramani > L: mpt-fusionlinux@avagotech.com Should this L: entry be changed? > L: linux-scsi@vger.kernel.org > W: http://www.lsilogic.com/support This W: link seems broken/dead. Should this be updated too? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 69/78] ncr5380: Fix whitespace in comments using regexp
On Sun, 2016-01-03 at 22:05 +, One Thousand Gnomes wrote: > On Sun, 03 Jan 2016 13:46:22 -0800 > Joe Perches wrote: > > > (using an email address for James that shouldn't bounce) > > > > On Sun, 2016-01-03 at 21:29 +, One Thousand Gnomes wrote: > > > > > I would beg to differ. As a tool for understanding the > > > > > differences as you > > > > > step through the versions it's invaluable. > > > > > > > > This removes intentional formatting that > > > > makes reading comments easier. > > > > > > And this matters at the end of the patch series because ? > > > > It removes intentional formatting that makes > > reading comments easier. > > I'm missing something here - the final result of the merge is completely > and easily readable. Likewise I can work through it and review the code > and compare them for the platform I care about (Mac68K) > > > And diff -w still works just fine. > > So you think 70+ patches should be redone because of that, when the end > result is a clean driver anyway ??? Nope, I think patch 69 should simply not be applied. I don't know and don't care if patches 70 through 78 depend on any changes made by patch 69. I'd expect no dependencies exist though. It doesn't matter much in any case. cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 69/78] ncr5380: Fix whitespace in comments using regexp
(using an email address for James that shouldn't bounce) On Sun, 2016-01-03 at 21:29 +, One Thousand Gnomes wrote: > > > I would beg to differ. As a tool for understanding the > > > differences as you > > > step through the versions it's invaluable. > > > > This removes intentional formatting that > > makes reading comments easier. > > And this matters at the end of the patch series because ? It removes intentional formatting that makes reading comments easier. And diff -w still works just fine. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 69/78] ncr5380: Fix whitespace in comments using regexp
On Sun, 2016-01-03 at 14:10 +, One Thousand Gnomes wrote:
> On Sat, 02 Jan 2016 23:54:28 -0800
> Joe Perches wrote:
>
> > On Sun, 2016-01-03 at 16:06 +1100, Finn Thain wrote:
> > > Hanging indentation was a poor choice for the text inside comments. It
> > > has been used in the wrong places and done badly elsewhere. There is
> > > little consistency within any file. One fork of the core driver uses
> > > tabs for this indentation while the other uses spaces. Better to use
> > > flush-left alignment throughout.
> > >
> > > This patch is the result of the following substitution. It replaces tabs
> > > and spaces at the start of a comment line with a single space.
> > >
> > > perl -i -pe 's,^(\t*[/ ]\*)[ \t]+,$1 ,' drivers/scsi/{atari_,}NCR5380.c
> > >
> > > This removes some unimportant discrepancies between the two core driver
> > > forks so that the important ones become obvious, to facilitate
> > > reunification.
> >
> > I still think this patch is poor at best and
> > overall not useful.
>
> I would beg to differ. As a tool for understanding the differences as you
> step through the versions it's invaluable.
This removes intentional formatting that
makes reading comments easier.
diff -w works well.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 69/78] ncr5380: Fix whitespace in comments using regexp
On Sun, 2016-01-03 at 16:06 +1100, Finn Thain wrote:
> Hanging indentation was a poor choice for the text inside comments. It
> has been used in the wrong places and done badly elsewhere. There is
> little consistency within any file. One fork of the core driver uses
> tabs for this indentation while the other uses spaces. Better to use
> flush-left alignment throughout.
>
> This patch is the result of the following substitution. It replaces tabs
> and spaces at the start of a comment line with a single space.
>
> perl -i -pe 's,^(\t*[/ ]\*)[ \t]+,$1 ,' drivers/scsi/{atari_,}NCR5380.c
>
> This removes some unimportant discrepancies between the two core driver
> forks so that the important ones become obvious, to facilitate
> reunification.
I still think this patch is poor at best and
overall not useful.
@@ -32,15 +32,15 @@
> /*
> * Further development / testing that should be done :
> * 1. Cleanup the NCR5380_transfer_dma function and DMA operation complete
> - * code so that everything does the same thing that's done at the
> - * end of a pseudo-DMA read operation.
> + * code so that everything does the same thing that's done at the
> + * end of a pseudo-DMA read operation.
> *
> * 2. Fix REAL_DMA (interrupt driven, polled works fine) -
> - * basically, transfer size needs to be reduced by one
> - * and the last byte read as is done with PSEUDO_DMA.
> + * basically, transfer size needs to be reduced by one
> + * and the last byte read as is done with PSEUDO_DMA.
> *
> * 4. Test SCSI-II tagged queueing (I have no devices which support
> - * tagged queueing)
> + * tagged queueing)
Numbering 1, 2, then 4 could be improved.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Wed, 2015-12-23 at 13:03 +1100, Finn Thain wrote: > On Tue, 22 Dec 2015, Joe Perches wrote: > > > On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > > > On Tue, 22 Dec 2015, Joe Perches wrote: > > > > > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > > > This patch is just the result of two substitutions. The first > > > > > removes any tabs and spaces at the end of the line. The second > > > > > replaces runs of tabs and spaces at the beginning of comment lines > > > > > with a single space. > > > > > > > > I think the second of these isn't done well. > > > > > > The aim of this patch is not to fix code style, but to make it > > > possible to compare these two files so that the fork can be repaired. > > > Regexp is very helpful in creating uniformity (and a minimal diff). > > > > > > If this was a coding style issue, we would be discussing the use of > > > kernel-doc format for the affected comments, not whitespace. > > > > > > > Many of these comments post reformatting are much worse to read > > > > because of lost alignment. > > > > > > You exaggerate a very trivial point. > > > > > > > > I prefer that all patches be improvements. > > > > Agreed. But the example you cited is an improvement, in that it creates > consistency. I think "consistency" isn't a useful argument. The kernel code doesn't care about any other external code bases. > Like you, I prefer to see formal parameters aligned when wrapped. But this > isn't a formal parameter list, it is a comment, and no comment should > duplicate code. > > Can you suggest a better regexp? Since this is patch 68 in the series, > there is a good chance that it will need to be regenerated. I suggest you do 2 patches here. One that removes unnecessary trailing spaces and converts multiple leading spaces to tabs where appropriate and a second patch that fixes whatever odd indentation that does exist after comment leading *. I think there aren't many instances of those and I think those should be done by hand rather than regex. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > On Tue, 22 Dec 2015, Joe Perches wrote: > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > This patch is just the result of two substitutions. The first removes > > > any tabs and spaces at the end of the line. The second replaces runs > > > of tabs and spaces at the beginning of comment lines with a single > > > space. > > > > I think the second of these isn't done well. > > The aim of this patch is not to fix code style, but to make it possible to > compare these two files so that the fork can be repaired. Regexp is very > helpful in creating uniformity (and a minimal diff). > > If this was a coding style issue, we would be discussing the use of > kernel-doc format for the affected comments, not whitespace. > > > Many of these comments post reformatting are > > much worse to read because of lost alignment. > > You exaggerate a very trivial point. I prefer that all patches be improvements. > I admit that a small proportion of comments are slightly less readable. > But it is the diff that needs to be readable in order to resolve the fork. diff -w works well. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > This patch is just the result of two substitutions. The first removes any > tabs and spaces at the end of the line. The second replaces runs of > tabs and spaces at the beginning of comment lines with a single space. I think the second of these isn't done well. Many of these comments post reformatting are much worse to read because of lost alignment. For instance: > +/* > * Function : int NCR5380_select(struct Scsi_Host *instance, > - * struct scsi_cmnd *cmd) > + * struct scsi_cmnd *cmd) > * > * Purpose : establishes I_T_L or I_T_L_Q nexus for new or existing command, > - * including ARBITRATION, SELECTION, and initial message out for > - * IDENTIFY and queue messages. > + * including ARBITRATION, SELECTION, and initial message out for > + * IDENTIFY and queue messages. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [SCSI] osd: fix signed char versus %02x issue
On Thu, 2015-12-10 at 14:13 -0500, Martin K. Petersen wrote: > > > > > > "Andy" == Andy Shevchenko writes: > > Andy> I have several patches on SCSI subsytem like this one. Some of > Andy> them didn't manage kernel (even having Ack!) for years already. > Andy> Is it okay if I collect them together and send a bunch once again > > Re-sending to linux-scsi is fine. The trick is finding people willing to > review them... Generally speaking, it hasn't been for lack of review. SCSI has been one of the slowest subsystems to apply simple defect correction (not whitespace style) patches. Mostly these patches have been ignored. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] storvsc: add more logging for error and warning messages
On Thu, 2015-12-03 at 19:47 -0800, Long Li wrote:
> Introduce a logging level for storvsc to log certain error/warning
> messages. Those messages are helpful in some environments, e.g.
> Microsoft Azure, for customer support and troubleshooting purposes.
[]
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
[]
> +static inline bool do_logging(int level)
> +{
> + return (logging_level >= level) ? true : false;
The ternary is not necessary
return logging_level >= level;
is enough
> +}
> +
> +
> struct vmscsi_win8_extension {
> /*
> * The following were added in Windows 8
> @@ -1183,7 +1198,7 @@ static void storvsc_command_completion(struct
> storvsc_cmd_request *cmd_request)
>
> scmnd->result = vm_srb->scsi_status;
>
> - if (scmnd->result) {
> + if (scmnd->result && do_logging(STORVSC_LOGGING_ERROR)) {
> if (scsi_normalize_sense(scmnd->sense_buffer,
> SCSI_SENSE_BUFFERSIZE, &sense_hdr))
> scsi_print_sense_hdr(scmnd->device, "storvsc",
Is it appropriate to make this scsi_normalize_sense call
conditional on do_logging here?
> @@ -1239,12 +1254,25 @@ static void storvsc_on_io_completion(struct hv_device
> *device,
> stor_pkt->vm_srb.sense_info_length =
> vstor_packet->vm_srb.sense_info_length;
>
> + if (vstor_packet->vm_srb.scsi_status != 0 ||
> + vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
> + if (do_logging(STORVSC_LOGGING_WARN))
> + dev_warn(&device->device,
> + "cmd 0x%x scsi status 0x%x srb status 0x%x\n",
> + stor_pkt->vm_srb.cdb[0],
> + vstor_packet->vm_srb.scsi_status,
> + vstor_packet->vm_srb.srb_status);
It might make some sense to use another macro indirection like
#define svc_log_warn(dev, level, fmt, ...) \
do {\
if (do_logging(STORSVC_LOGGING_##level) \
dev_warn(&(dev)->device, fmt, ##__VA_ARGS__); \
} while (0)
So a use could be:
if (vstore_packet...)
svc_log_warn(device, WARN, ...);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] arcmsr: Split dma resource allocation to a new function
On Thu, 2015-11-26 at 19:41 +0800, Ching Huang wrote:
> split dma resource allocation and io register assignment from get_config to a
> new function arcmsr_alloc_io_queue.
trivia:
> diff -uprN a/drivers/scsi/arcmsr/arcmsr_hba.c
> b/drivers/scsi/arcmsr/arcmsr_hba.c
[]
> +static bool arcmsr_alloc_io_queue(struct AdapterControlBlock *acb)
> +{
[]
> + dma_coherent = dma_alloc_coherent(&pdev->dev,
> acb->roundup_ccbsize,
> + &dma_coherent_handle, GFP_KERNEL);
> + if (!dma_coherent){
> + pr_notice("arcmsr%d: DMA allocation failed.\n",
> acb->host->host_no);
> + return false;
> + }
> + memset(dma_coherent, 0, acb->roundup_ccbsize);
>
There is a dma_zalloc_coherent
(and even more trivially)
Most all of your error messages don't use periods.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
