Re: [RFC 00/22] target: se_node_acl LUN list RCU conversion
On Mon, 2015-04-06 at 23:17 -0700, Christoph Hellwig wrote: On Thu, Apr 02, 2015 at 01:57:10PM -0700, Nicholas A. Bellinger wrote: mempools are for I/O path that make guaranteed progress. While the callers of core_enable_device_list_for_node are in the control path, and not in a very deep callstack, fairly shallow below the configfs interface. This is not a use case for mempools, as there is no need to guarantee progress in deep I/O callstacks. I was more concerned about the creation of se_dev_entry for dynamically generated se_node_acl in the login path. Having to back out everything upon se_dev_entry allocation failure is annoying. But unless you specificly pre-create the number of entries that core_dev_add_lun may create that's the only way to handle it. mempool rely on the fact the objects get freed to the mempool again to make forward progress, which isn't the case for a lon-lived se_dev_entry allocation. -- Point taken. Dropping the extra mempool_t usage for this case. --nab -- 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 13/14] ppdev: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
drivers/char/ppdev.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index ae0b42b..14374d7 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -748,10 +748,14 @@ static const struct file_operations pp_fops = {
.release= pp_release,
};
-static void pp_attach(struct parport *port)
+static int pp_attach(struct parport *port)
{
- device_create(ppdev_class, port-dev, MKDEV(PP_MAJOR, port-number),
- NULL, parport%d, port-number);
+ struct device *dev;
+
+ dev = device_create(ppdev_class, port-dev,
+ MKDEV(PP_MAJOR, port-number), NULL,
+ parport%d, port-number);
+ return PTR_ERR_OR_ZERO(dev);
}
static void pp_detach(struct parport *port)
--
1.8.1.2
--
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 11/14] net: plip: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
also return the proper error code in module_init.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
drivers/net/plip/plip.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c
index 040b897..6706bc3 100644
--- a/drivers/net/plip/plip.c
+++ b/drivers/net/plip/plip.c
@@ -1243,7 +1243,7 @@ plip_searchfor(int list[], int a)
/* plip_attach() is called (by the parport code) when a port is
* available to use. */
-static void plip_attach (struct parport *port)
+static int plip_attach(struct parport *port)
{
static int unit;
struct net_device *dev;
@@ -1254,13 +1254,13 @@ static void plip_attach (struct parport *port)
plip_searchfor(parport, port-number)) {
if (unit == PLIP_MAX) {
printk(KERN_ERR plip: too many devices\n);
- return;
+ return -EINVAL;
}
sprintf(name, plip%d, unit);
dev = alloc_etherdev(sizeof(struct net_local));
if (!dev)
- return;
+ return -ENOMEM;
strcpy(dev-name, name);
@@ -1300,12 +1300,13 @@ static void plip_attach (struct parport *port)
dev-name, dev-base_addr);
dev_plip[unit++] = dev;
}
- return;
+ return 0;
err_parport_unregister:
parport_unregister_device(nl-pardev);
err_free_dev:
free_netdev(dev);
+ return -ENODEV;
}
/* plip_detach() is called (by the parport code) when a port is
@@ -1379,6 +1380,8 @@ __setup(plip=, plip_setup);
static int __init plip_init (void)
{
+ int err;
+
if (parport[0] == -2)
return 0;
@@ -1387,9 +1390,10 @@ static int __init plip_init (void)
timid = 0;
}
- if (parport_register_driver (plip_driver)) {
+ err = parport_register_driver(plip_driver);
+ if (err) {
printk (KERN_WARNING plip: couldn't register driver\n);
- return 1;
+ return err;
}
return 0;
--
1.8.1.2
--
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 12/14] i2c-parport: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
drivers/i2c/busses/i2c-parport.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
index a1fac5a..761a775 100644
--- a/drivers/i2c/busses/i2c-parport.c
+++ b/drivers/i2c/busses/i2c-parport.c
@@ -160,14 +160,14 @@ static void i2c_parport_irq(void *data)
SMBus alert received but no ARA client!\n);
}
-static void i2c_parport_attach(struct parport *port)
+static int i2c_parport_attach(struct parport *port)
{
struct i2c_par *adapter;
adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL);
if (adapter == NULL) {
printk(KERN_ERR i2c-parport: Failed to kzalloc\n);
- return;
+ return -ENOMEM;
}
pr_debug(i2c-parport: attaching to %s\n, port-name);
@@ -230,13 +230,14 @@ static void i2c_parport_attach(struct parport *port)
mutex_lock(adapter_list_lock);
list_add_tail(adapter-node, adapter_list);
mutex_unlock(adapter_list_lock);
- return;
+ return 0;
err_unregister:
parport_release(adapter-pdev);
parport_unregister_device(adapter-pdev);
err_free:
kfree(adapter);
+ return -ENODEV;
}
static void i2c_parport_detach(struct parport *port)
--
1.8.1.2
--
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 10/14] pps: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
drivers/pps/clients/pps_parport.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/pps/clients/pps_parport.c
b/drivers/pps/clients/pps_parport.c
index 38a8bbe..a411621 100644
--- a/drivers/pps/clients/pps_parport.c
+++ b/drivers/pps/clients/pps_parport.c
@@ -134,7 +134,7 @@ out_both:
return;
}
-static void parport_attach(struct parport *port)
+static int parport_attach(struct parport *port)
{
struct pps_client_pp *device;
struct pps_source_info info = {
@@ -151,7 +151,7 @@ static void parport_attach(struct parport *port)
device = kzalloc(sizeof(struct pps_client_pp), GFP_KERNEL);
if (!device) {
pr_err(memory allocation failed, not attaching\n);
- return;
+ return -ENOMEM;
}
device-pardev = parport_register_device(port, KBUILD_MODNAME,
@@ -179,7 +179,7 @@ static void parport_attach(struct parport *port)
pr_info(attached to %s\n, port-name);
- return;
+ return 0;
err_release_dev:
parport_release(device-pardev);
@@ -187,6 +187,7 @@ err_unregister_dev:
parport_unregister_device(device-pardev);
err_free:
kfree(device);
+ return -ENODEV;
}
static void parport_detach(struct parport *port)
--
1.8.1.2
--
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 01/14] parport: return value of attach and parport_register_driver
On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote: 1) We can't apply this patch on its own so this way of breaking up the patches doesn't work. yes, if the first patch is reverted for any reason all the others need to be reverted also. so then everything in one single patch? 2) I was thinking that all the -attach() calls would have to succeed or we would bail. Having some of them succeed and some fail doesn't seem like it will simplify the driver code very much. But I can also see your point. Hm... to clarify my point more here: any system might have more than one parallel port but the module might decide to use just one. so in that case attach will return 0 for the port that it wishes to use, for others it will be a error code. So in parport_register_driver if we get error codes in all the attach calls then we know that attach has definitely failed, but atleast one 0 means one attach call has succeeded, which will happen in case of staging/panel, net/plip... Minor comment: No need to preserve the error code if there are lots which we miss. We may as well hard code an error code. But that's a minor thing. Does this actually simplify the driver code? That's the more important thing. i don't think this will simplify the driver code, but atleast now parport_register_driver() will not report success when we have actually failed. And as a result module_init will also fail which is supposed to be the actual behviour. regards sudip regards, dan carpenter -- 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 00/14] parport: check success of attach call
Currently we are not checking if attach has succeeded or not. Also parport_register_driver() always return 0 even if attach fails. But in many places where attach has been used, it can fail. So, modified the parport code to check the return value of attach and accordingly return either 0 or error code from parport_register_driver. Sudip Mukherjee (14): parport: return value of attach and parport_register_driver ALSA: portman2x4: return proper error values from attach ALSA: mts64: return proper error values from attach staging: panel: return proper error values from attach spi: lm70llp: return proper error values from attach spi: butterfly: return proper error values from attach [SCSI] ppa: return proper error values from attach [SCSI] imm: return proper error values from attach pps: return proper error values from attach pps: return proper error values from attach net: plip: return proper error values from attach i2c-parport: return proper error values from attach ppdev: return proper error values from attach char: lp: return proper error values from attach drivers/char/lp.c| 16 +++- drivers/char/ppdev.c | 10 +++--- drivers/i2c/busses/i2c-parport.c | 7 --- drivers/net/plip/plip.c | 16 ++-- drivers/parport/share.c | 20 +++- drivers/pps/clients/pps_parport.c| 7 --- drivers/pps/generators/pps_gen_parport.c | 9 + drivers/scsi/imm.c | 4 ++-- drivers/scsi/ppa.c | 4 ++-- drivers/spi/spi-butterfly.c | 7 --- drivers/spi/spi-lm70llp.c| 7 --- drivers/staging/panel/panel.c| 11 ++- include/linux/parport.h | 2 +- sound/drivers/mts64.c| 13 - sound/drivers/portman2x4.c | 15 ++- 15 files changed, 93 insertions(+), 55 deletions(-) -- 1.8.1.2 -- 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 08/14] [SCSI] imm: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
drivers/scsi/imm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/imm.c b/drivers/scsi/imm.c
index 89a8266..e26330a 100644
--- a/drivers/scsi/imm.c
+++ b/drivers/scsi/imm.c
@@ -1225,9 +1225,9 @@ out:
return err;
}
-static void imm_attach(struct parport *pb)
+static int imm_attach(struct parport *pb)
{
- __imm_attach(pb);
+ return __imm_attach(pb);
}
static void imm_detach(struct parport *pb)
--
1.8.1.2
--
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 14/14] char: lp: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
drivers/char/lp.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index c4094c4..6988480 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -900,34 +900,40 @@ static int lp_register(int nr, struct parport *port)
return 0;
}
-static void lp_attach (struct parport *port)
+static int lp_attach(struct parport *port)
{
unsigned int i;
+ int ret = -ENODEV;
switch (parport_nr[0]) {
case LP_PARPORT_UNSPEC:
case LP_PARPORT_AUTO:
if (parport_nr[0] == LP_PARPORT_AUTO
port-probe_info[0].class != PARPORT_CLASS_PRINTER)
- return;
+ return ret;
if (lp_count == LP_NO) {
printk(KERN_INFO lp: ignoring parallel port (max.
%d)\n,LP_NO);
- return;
+ return ret;
}
- if (!lp_register(lp_count, port))
+ if (!lp_register(lp_count, port)) {
lp_count++;
+ ret = 0;
+ }
break;
default:
for (i = 0; i LP_NO; i++) {
if (port-number == parport_nr[i]) {
- if (!lp_register(i, port))
+ if (!lp_register(i, port)) {
lp_count++;
+ ret = 0;
+ }
break;
}
}
break;
}
+ return ret;
}
static void lp_detach (struct parport *port)
--
1.8.1.2
--
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 09/14] pps: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
drivers/pps/generators/pps_gen_parport.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/pps/generators/pps_gen_parport.c
b/drivers/pps/generators/pps_gen_parport.c
index dcd39fb..499f410 100644
--- a/drivers/pps/generators/pps_gen_parport.c
+++ b/drivers/pps/generators/pps_gen_parport.c
@@ -190,18 +190,18 @@ static inline ktime_t next_intr_time(struct
pps_generator_pp *dev)
dev-port_write_time + 3 * SAFETY_INTERVAL));
}
-static void parport_attach(struct parport *port)
+static int parport_attach(struct parport *port)
{
if (attached) {
/* we already have a port */
- return;
+ return -EALREADY;
}
device.pardev = parport_register_device(port, KBUILD_MODNAME,
NULL, NULL, NULL, PARPORT_FLAG_EXCL, device);
if (!device.pardev) {
pr_err(couldn't register with %s\n, port-name);
- return;
+ return -ENODEV;
}
if (parport_claim_or_block(device.pardev) 0) {
@@ -218,10 +218,11 @@ static void parport_attach(struct parport *port)
device.timer.function = hrtimer_event;
hrtimer_start(device.timer, next_intr_time(device), HRTIMER_MODE_ABS);
- return;
+ return 0;
err_unregister_dev:
parport_unregister_device(device.pardev);
+ return -ENODEV;
}
static void parport_detach(struct parport *port)
--
1.8.1.2
--
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 01/14] parport: return value of attach and parport_register_driver
On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote:
1) We can't apply this patch on its own so this way of breaking up the
patches doesn't work.
The right thing is to do add an attach_ret().
static int do_attach(drv)
{
if (drv-attach_ret)
return drv-attach_ret();
drv-attach();
return 0;
}
Then we convert one driver to use the new function pointer and see if
it simplifies the code. If so we can transition the others as well. If
not then we give up.
regards,
dan carpenter
--
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 01/14] parport: return value of attach and parport_register_driver
On Wed, Apr 08, 2015 at 02:44:37PM +0300, Dan Carpenter wrote: On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote: Then we convert one driver to use the new function pointer and see if it simplifies the code. If so we can transition the others as well. If not then we give up. i am sending a v2 and also a patch to change one driver. regards sudip regards, dan carpenter -- 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 02/14] ALSA: portman2x4: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
sound/drivers/portman2x4.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
index 464385a..866adbb 100644
--- a/sound/drivers/portman2x4.c
+++ b/sound/drivers/portman2x4.c
@@ -672,32 +672,37 @@ static int snd_portman_probe_port(struct parport *p)
return res ? -EIO : 0;
}
-static void snd_portman_attach(struct parport *p)
+static int snd_portman_attach(struct parport *p)
{
struct platform_device *device;
+ int ret;
device = platform_device_alloc(PLATFORM_DRIVER, device_count);
if (!device)
- return;
+ return -ENOMEM;
/* Temporary assignment to forward the parport */
platform_set_drvdata(device, p);
- if (platform_device_add(device) 0) {
+ ret = platform_device_add(device);
+
+ if (ret 0) {
platform_device_put(device);
- return;
+ return ret;
}
/* Since we dont get the return value of probe
* We need to check if device probing succeeded or not */
if (!platform_get_drvdata(device)) {
platform_device_unregister(device);
- return;
+ return -ENODEV;
}
/* register device in global table */
platform_devices[device_count] = device;
device_count++;
+
+ return 0;
}
static void snd_portman_detach(struct parport *p)
--
1.8.1.2
--
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 01/14] parport: return value of attach and parport_register_driver
1) We can't apply this patch on its own so this way of breaking up the patches doesn't work. 2) I was thinking that all the -attach() calls would have to succeed or we would bail. Having some of them succeed and some fail doesn't seem like it will simplify the driver code very much. But I can also see your point. Hm... Minor comment: No need to preserve the error code if there are lots which we miss. We may as well hard code an error code. But that's a minor thing. Does this actually simplify the driver code? That's the more important thing. regards, dan carpenter -- 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 04/14] staging: panel: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
drivers/staging/panel/panel.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index ea54fb4..61f6961 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -2188,15 +2188,15 @@ static struct notifier_block panel_notifier = {
0
};
-static void panel_attach(struct parport *port)
+static int panel_attach(struct parport *port)
{
if (port-number != parport)
- return;
+ return -ENODEV;
if (pprt) {
pr_err(%s: port-number=%d parport=%d, already registered!\n,
__func__, port-number, parport);
- return;
+ return -EALREADY;
}
pprt = parport_register_device(port, panel, NULL, NULL, /* pf, kf */
@@ -2206,7 +2206,7 @@ static void panel_attach(struct parport *port)
if (pprt == NULL) {
pr_err(%s: port-number=%d parport=%d,
parport_register_device() failed\n,
__func__, port-number, parport);
- return;
+ return -ENODEV;
}
if (parport_claim(pprt)) {
@@ -2230,7 +2230,7 @@ static void panel_attach(struct parport *port)
goto err_lcd_unreg;
}
register_reboot_notifier(panel_notifier);
- return;
+ return 0;
err_lcd_unreg:
if (lcd.enabled)
@@ -2238,6 +2238,7 @@ err_lcd_unreg:
err_unreg_device:
parport_unregister_device(pprt);
pprt = NULL;
+ return -ENODEV;
}
static void panel_detach(struct parport *port)
--
1.8.1.2
--
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 05/14] spi: lm70llp: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
drivers/spi/spi-lm70llp.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-lm70llp.c b/drivers/spi/spi-lm70llp.c
index ba72347..9b0fde1 100644
--- a/drivers/spi/spi-lm70llp.c
+++ b/drivers/spi/spi-lm70llp.c
@@ -190,7 +190,7 @@ static u32 lm70_txrx(struct spi_device *spi, unsigned
nsecs, u32 word, u8 bits)
return bitbang_txrx_be_cpha0(spi, nsecs, 0, 0, word, bits);
}
-static void spi_lm70llp_attach(struct parport *p)
+static int spi_lm70llp_attach(struct parport *p)
{
struct pardevice*pd;
struct spi_lm70llp *pp;
@@ -201,7 +201,7 @@ static void spi_lm70llp_attach(struct parport *p)
printk(KERN_WARNING
%s: spi_lm70llp instance already loaded. Aborting.\n,
DRVNAME);
- return;
+ return -EALREADY;
}
/* TODO: this just _assumes_ a lm70 is there ... no probe;
@@ -281,7 +281,7 @@ static void spi_lm70llp_attach(struct parport *p)
pp-spidev_lm70-bits_per_word = 8;
lm70llp = pp;
- return;
+ return 0;
out_bitbang_stop:
spi_bitbang_stop(pp-bitbang);
@@ -296,6 +296,7 @@ out_free_master:
(void) spi_master_put(master);
out_fail:
pr_info(%s: spi_lm70llp probe fail, status %d\n, DRVNAME, status);
+ return status;
}
static void spi_lm70llp_detach(struct parport *p)
--
1.8.1.2
--
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 01/14] parport: return value of attach and parport_register_driver
as of now, we are not checking if attach or parport_register_driver
has succeeded or failed. But attach can fail in the places where they
have been used. Lets check the return of attach, and if attach fails
then parport_register_driver should also fail. We can have multiple
parallel port so we only mark attach as failed only if it has never
returned a 0.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
drivers/parport/share.c | 20 +++-
include/linux/parport.h | 2 +-
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index 3fa6624..640ce41 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -148,23 +148,33 @@ static void get_lowlevel_driver (void)
* callback, but if the driver wants to take a copy of the
* pointer it must call parport_get_port() to do so.
*
- * Returns 0 on success. Currently it always succeeds.
+ * Returns 0 on success.
**/
int parport_register_driver (struct parport_driver *drv)
{
struct parport *port;
+ int ret, err;
+ bool attached = false;
if (list_empty(portlist))
get_lowlevel_driver ();
mutex_lock(registration_lock);
- list_for_each_entry(port, portlist, list)
- drv-attach(port);
- list_add(drv-list, drivers);
+ list_for_each_entry(port, portlist, list) {
+ err = drv-attach(port);
+ if (err == 0)
+ attached = true;
+ else
+ ret = err;
+ }
+ if (attached) {
+ list_add(drv-list, drivers);
+ ret = 0;
+ }
mutex_unlock(registration_lock);
- return 0;
+ return ret;
}
/**
diff --git a/include/linux/parport.h b/include/linux/parport.h
index c22f125..9411065 100644
--- a/include/linux/parport.h
+++ b/include/linux/parport.h
@@ -249,7 +249,7 @@ struct parport {
struct parport_driver {
const char *name;
- void (*attach) (struct parport *);
+ int (*attach)(struct parport *);
void (*detach) (struct parport *);
struct list_head list;
};
--
1.8.1.2
--
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 03/14] ALSA: mts64: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
sound/drivers/mts64.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/sound/drivers/mts64.c b/sound/drivers/mts64.c
index 2a008a9..fb6223e 100644
--- a/sound/drivers/mts64.c
+++ b/sound/drivers/mts64.c
@@ -874,32 +874,35 @@ static int snd_mts64_probe_port(struct parport *p)
return res;
}
-static void snd_mts64_attach(struct parport *p)
+static int snd_mts64_attach(struct parport *p)
{
struct platform_device *device;
+ int ret;
device = platform_device_alloc(PLATFORM_DRIVER, device_count);
if (!device)
- return;
+ return -ENOMEM;
/* Temporary assignment to forward the parport */
platform_set_drvdata(device, p);
- if (platform_device_add(device) 0) {
+ ret = platform_device_add(device);
+ if (ret 0) {
platform_device_put(device);
- return;
+ return ret;
}
/* Since we dont get the return value of probe
* We need to check if device probing succeeded or not */
if (!platform_get_drvdata(device)) {
platform_device_unregister(device);
- return;
+ return -ENODEV;
}
/* register device in global table */
platform_devices[device_count] = device;
device_count++;
+ return 0;
}
static void snd_mts64_detach(struct parport *p)
--
1.8.1.2
--
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 07/14] [SCSI] ppa: return proper error values from attach
now that we are monitoring the return value from attach, make the
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
drivers/scsi/ppa.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ppa.c b/drivers/scsi/ppa.c
index 1db8b26..48898ec 100644
--- a/drivers/scsi/ppa.c
+++ b/drivers/scsi/ppa.c
@@ -1090,9 +1090,9 @@ out:
return err;
}
-static void ppa_attach(struct parport *pb)
+static int ppa_attach(struct parport *pb)
{
- __ppa_attach(pb);
+ return __ppa_attach(pb);
}
static void ppa_detach(struct parport *pb)
--
1.8.1.2
--
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 6/9] snic:Add low level queuing interfaces
Hi Hannes, Thank you for reviewing patches. Please find responses inline. On 07/04/15 12:08 pm, Hannes Reinecke h...@suse.de wrote: On 04/02/2015 10:13 AM, Narsimhulu Musini (nmusini) wrote: Hi Hannes, Thank you for reviewing the patch. Please find responses inline. I will incorporate the comments and suggestions in next patch submittal. On 25/03/15 4:43 pm, Hannes Reinecke h...@suse.de wrote: Hi Narsimhulu, On 03/11/2015 06:01 PM, Narsimhulu Musini wrote: These files contain low level queueing interfaces includes hardware queues, and management of hardware features. v2 driver supports x86-64 arch, so removed cpu_to_XX API to maintain consistency. Signed-off-by: Narsimhulu Musini nmus...@cisco.com Signed-off-by: Sesidhar Baddela sebad...@cisco.com --- Please find some comments below. [ .. ] That really looks like a 64bit structure read in as 32bit values ... If so please use 64bit values here ... The structure members are aligned to hardware data, I would prefer to keep it same. I _know_ that they are aligned. The point of my question was whether these values are _real_ 32-bit values or rather 64-bit values with the top 32-bit masked off. They are real 32 bit values. These are registers, falling on 64 bit boundaries. Cheers, Hannes -- Dr. Hannes Reinecke zSeries Storage h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) Thanks Narsimhulu -- 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 02/15] target: Add protected fabric + unprotected device support
On Tue, 2015-04-07 at 19:27 -0400, Martin K. Petersen wrote: nab == Nicholas A Bellinger n...@daterainc.com writes: nab This specifically is to allow LIO to perform WRITE_STRIP + nab READ_INSERT operations when functioning with non T10-PI enabled nab devices, seperate from any available hw offloads the fabric nab supports. How do you handle RDPROTECT/WRPROTECT values of 3 if the PI is not persistent? AFAICT, this would result in cmd-prot_op = TARGET_PROT_*_PASS and cmd-prot_checks = 0 for RDPROTECT/WRPROTECT == 0x3 in sbc_set_prot_op_checks() code. Do DOUT_STRIP + DIN_INSERT need to be called if a protection buffer is present when RDPROTECT/WRPROTECT == 0x3 if fabric_prot was cleared..? Or should the command be rejected when a protection buffer is present + RDPROTECT/WRPROTECT is non-zero if fabric_prot was cleared..? Also wrt to PI persistence across session restart, one way it can work is to store the last se_sess-sess_prot_type in se_node_acl, and reinstate the previous setting across session restart. This would allow new sessions to pickup the latest fabric_prot_type endpoint attribute value, but make existing ones with PI enabled keep their previous sess_prot_type settings. WDYT..? --nab -- 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 6/9] snic:Add low level queuing interfaces
On 04/08/2015 11:05 AM, Narsimhulu Musini (nmusini) wrote: Hi Hannes, Thank you for reviewing patches. Please find responses inline. On 07/04/15 12:08 pm, Hannes Reinecke h...@suse.de wrote: On 04/02/2015 10:13 AM, Narsimhulu Musini (nmusini) wrote: Hi Hannes, Thank you for reviewing the patch. Please find responses inline. I will incorporate the comments and suggestions in next patch submittal. On 25/03/15 4:43 pm, Hannes Reinecke h...@suse.de wrote: Hi Narsimhulu, On 03/11/2015 06:01 PM, Narsimhulu Musini wrote: These files contain low level queueing interfaces includes hardware queues, and management of hardware features. v2 driver supports x86-64 arch, so removed cpu_to_XX API to maintain consistency. Signed-off-by: Narsimhulu Musini nmus...@cisco.com Signed-off-by: Sesidhar Baddela sebad...@cisco.com --- Please find some comments below. [ .. ] That really looks like a 64bit structure read in as 32bit values ... If so please use 64bit values here ... The structure members are aligned to hardware data, I would prefer to keep it same. I _know_ that they are aligned. The point of my question was whether these values are _real_ 32-bit values or rather 64-bit values with the top 32-bit masked off. They are real 32 bit values. These are registers, falling on 64 bit boundaries. Right, that's okay then. Cheers, Hannes -- Dr. Hannes ReineckezSeries Storage h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) -- 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 1/9] snic: snic module infrastructure
Hi Christoph,
Thank you for reviewing the patch. Please find responses inline.
I will incorporate the comments and suggestions in next patch submittal.
On 02/04/15 2:51 pm, Christoph Hellwig h...@infradead.org wrote:
+/*
+ * Usage of the scsi_cmnd scratchpad.
+ * These fields are locked by the hashed req_lock.
+ */
+#define CMD_SP(Cmnd)((Cmnd)-SCp.ptr)
+#define CMD_STATE(Cmnd) ((Cmnd)-SCp.phase)
+#define CMD_ABTS_STATUS(Cmnd) ((Cmnd)-SCp.Message)
+#define CMD_LR_STATUS(Cmnd) ((Cmnd)-SCp.have_data_in)
+#define CMD_TAG(Cmnd) ((Cmnd)-SCp.sent_command)
+#define CMD_FLAGS(Cmnd) ((Cmnd)-SCp.Status)
Please don't abuse -SCp for new drivers. Declare your own structure
and set .cmd_size in the host template to it's size, then access it
using scsi_cmd_priv().
Sure, driver will use its own private structure.
That should also avoid the need for the mempool
that the driver currently creates.
Yes, agreed. For now, I will just consider making changes to replace
scratch buffer usage. And will take up the mempool creation in future
patches.
+static int
+snic_slave_alloc(struct scsi_device *sdev)
+{
+u32 qdepth = 0, max_ios = 0;
+struct snic_tgt *tgt = starget_to_tgt(scsi_target(sdev));
+
+if (!tgt || snic_tgt_chkready(tgt))
+return -ENXIO;
+
+max_ios = snic_max_qdepth;
+max_ios = snic_max_qdepth;
+qdepth = min_t(u32, max_ios, SNIC_MAX_QUEUE_DEPTH);
+scsi_change_queue_depth(sdev, qdepth);
Plase set the queue_depth in -slave_configure like all other drivers.
Sure, I will move it to -slave_configure.
+.cmd_per_lun = 3,
why so low?
Thanks for pointing this, will set it to default queue depth.
+
+#ifndef PCI_VENDOR_ID_CISCO
+#define PCI_VENDOR_ID_CISCO 0x1137
+#endif
Just use the numeric value directly.
Sure.
+
+#define SNIC_OS_TYPESNIC_OS_LINUX
+#define snic_cmd_tag(sc)(((struct scsi_cmnd *) sc)-request-tag)
+
+extern struct device_attribute *snic_attrs[];
+
+static inline struct kmem_cache *
+snic_cache_create(const char *cache_name, const int sz)
+{
+void *cp;
+
+cp = kmem_cache_create(cache_name, sz, SNIC_SG_DESC_ALIGN,
+ SLAB_HWCACHE_ALIGN, NULL);
+
+return (struct kmem_cache *) cp;
+}
Please remove all the wrappers in the snic_os.h file and use the Linux
APIs directly.
Sure, I will directly use the APIs, and delete the snic_os.h file.
Thanks
Narsimhulu
--
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 2/9] snic:Add interrupt, resource firmware interfaces
Hi Hannes,
Thank you for reviewing patches. Please find responses inline.
I will incorporate the comments and suggestions in next patch submittal.
On 07/04/15 11:57 am, Hannes Reinecke h...@suse.de wrote:
Hi Narsimhulu,
On 04/02/2015 09:48 AM, Narsimhulu Musini (nmusini) wrote:
Hi Hannes,
Thank you for reviewing patches. Please find responses inline.
I will incorporate the comments and suggestions in next patch submittal.
On 25/03/15 3:48 pm, Hannes Reinecke h...@suse.de wrote:
On 03/11/2015 06:01 PM, Narsimhulu Musini wrote:
[ .. ]
+void
+snic_free_intr(struct snic *snic)
+{
+ int i;
+
+ /* ONLY interrupt mode MSIX is supported */
+ for (i = 0; i ARRAY_SIZE(snic-msix); i++) {
+ if (snic-msix[i].requested) {
+ free_irq(snic-msix_entry[i].vector,
+ snic-msix[i].devid);
+ }
+ }
+} /* end of snic_free_intr */
+
+int
+snic_request_intr(struct snic *snic)
+{
+ int ret = 0, i;
+
+#ifdef SNIC_DEBUG
+ enum vnic_dev_intr_mode intr_mode;
+
+ intr_mode = vnic_dev_get_intr_mode(snic-vdev);
+ SNIC_BUG_ON(intr_mode != VNIC_DEV_INTR_MODE_MSIX);
+#endif
+
+ /* FIXME: Pass devid as work queue or completion queue pointers
+ * except for err_notify
+ */
+ sprintf(snic-msix[SNIC_MSIX_WQ].devname,
+ %.11s-scsi-wq,
+ snic-name);
Any reason why you didn't fix it now?
This is a new submission, so I would have expected
_you_ are fine with the code ...
The comment is a place holder for future changes when hardware supports
multiple queues. one idea is to pass queue pointer and retrieve snic
from
queue pointer. At this moment, hardware provides single queue. So
directly
passing snic.
+ snic-msix[SNIC_MSIX_WQ].isr = snic_isr_msix_wq;
+ snic-msix[SNIC_MSIX_WQ].devid = snic;
+
+ /* FIXME: name can be scsi_cq or iocmpl */
+ sprintf(snic-msix[SNIC_MSIX_IO_CMPL].devname,
+ %.11s-io-cmpl,
+ snic-name);
Same here.
I would accept FIXMEs is if they require an infrastructure
change or if it refers to future / planned
hardware updates. This doesn't strike me as either ...
The comment is a place holder for future changes when hardware supports
multiple queues. one idea is to pass queue pointer and retrieve snic
from
queue pointer. At this moment, hardware provides single queue. So
directly
passing snic.
Okay, can you then please update the comment indicating this?
It's not directly a 'FIXME', as the current hardware/firmware
doesn't support this.
At the same time I see the value of having this comment in there.
Sure, I will update.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries Storage
h...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
Thanks
Narsimhulu
--
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 01/14] parport: return value of attach and parport_register_driver
On Wed, Apr 08, 2015 at 05:20:10PM +0530, Sudip Mukherjee wrote: On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote: 1) We can't apply this patch on its own so this way of breaking up the patches doesn't work. yes, if the first patch is reverted for any reason all the others need to be reverted also. so then everything in one single patch? The problem is that patch 1/1 breaks the build. The rule is that we should be able to apply part of a patch series and nothing breaks. If we apply the patch series out of order than things break that's our problem, yes. But if we apply only 1/1 and it breaks, that's a problem with the series. 2) I was thinking that all the -attach() calls would have to succeed or we would bail. Having some of them succeed and some fail doesn't seem like it will simplify the driver code very much. But I can also see your point. Hm... My other issue with this patch series which is related to #2 is that it's not clear that anyone is checking the return value and doing correct things with it. Hopefully, when we use the attach_ret() approach then it will be more clear if/how the return value is useful. regards, dan carpenter -- 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 02/14] ALSA: portman2x4: return proper error values from attach
On Wed, Apr 08, 2015 at 04:32:57PM +0300, Sergei Shtylyov wrote: Hello. On 4/8/2015 2:20 PM, Sudip Mukherjee wrote: now that we are monitoring the return value from attach, make the So you've first changed the method prototype and follow up with the changes to the actual implementations? That's backward. I'm afraid such changes can't be done piecemeal. Dan Carpenter has exlained me the problem with this aproach. I have already sent a v2 which doesnot break anything and if everyone approves that way then we can change one driver at a time and nothing will break. regards sudip -- 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 02/14] ALSA: portman2x4: return proper error values from attach
Hello.
On 4/8/2015 2:20 PM, Sudip Mukherjee wrote:
now that we are monitoring the return value from attach, make the
So you've first changed the method prototype and follow up with the
changes to the actual implementations? That's backward. I'm afraid such
changes can't be done piecemeal.
required changes to return proper value from its attach function.
Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
sound/drivers/portman2x4.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
index 464385a..866adbb 100644
--- a/sound/drivers/portman2x4.c
+++ b/sound/drivers/portman2x4.c
@@ -672,32 +672,37 @@ static int snd_portman_probe_port(struct parport *p)
return res ? -EIO : 0;
}
-static void snd_portman_attach(struct parport *p)
+static int snd_portman_attach(struct parport *p)
{
struct platform_device *device;
+ int ret;
device = platform_device_alloc(PLATFORM_DRIVER, device_count);
if (!device)
- return;
+ return -ENOMEM;
/* Temporary assignment to forward the parport */
platform_set_drvdata(device, p);
- if (platform_device_add(device) 0) {
+ ret = platform_device_add(device);
+
I don't think empty line is needed here.
+ if (ret 0) {
platform_device_put(device);
- return;
+ return ret;
}
[...]
WBR, Sergei
--
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 2/2] target/rd: Don't pass imcomplete scatterlist entries to sbc_dif_verify_*
2015-04-08 14:13 GMT+09:00 Nicholas A. Bellinger n...@linux-iscsi.org:
On Sun, 2015-04-05 at 23:59 +0900, Akinobu Mita wrote:
The scatterlist for protection information which is passed to
sbc_dif_verify_read() or sbc_dif_verify_write() requires that
neighboring scatterlist entries are contiguous or chained so that they
can be iterated by sg_next().
However, the protection information for RD-MCP backends could be located
in the multiple scatterlist arrays when the ramdisk space is too large.
So if the read/write request straddles this boundary, sbc_dif_verify_read()
or sbc_dif_verify_write() can't iterate all scatterlist entries.
This fixes it by allocating temporary scatterlist if it is needed.
Signed-off-by: Akinobu Mita akinobu.m...@gmail.com
Cc: Nicholas Bellinger n...@linux-iscsi.org
Cc: Sagi Grimberg sa...@dev.mellanox.co.il
Cc: Martin K. Petersen martin.peter...@oracle.com
Cc: Christoph Hellwig h...@lst.de
Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com
Cc: target-de...@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
---
* No change from v1
drivers/target/target_core_rd.c | 39 +++
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/drivers/target/target_core_rd.c
b/drivers/target/target_core_rd.c
index ac5e8d2..6e25eaa 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -390,11 +390,12 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd
*cmd, dif_verify dif_verify)
struct se_device *se_dev = cmd-se_dev;
struct rd_dev *dev = RD_DEV(se_dev);
struct rd_dev_sg_table *prot_table;
+ bool need_to_release = false;
struct scatterlist *prot_sg;
u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size;
- u32 prot_offset, prot_page;
+ u32 prot_offset, prot_page, prot_npages;
u64 tmp;
- sense_reason_t rc;
+ sense_reason_t rc = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
tmp = cmd-t_task_lba * se_dev-prot_length;
prot_offset = do_div(tmp, PAGE_SIZE);
@@ -404,10 +405,40 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd
*cmd, dif_verify dif_verify)
if (!prot_table)
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
- prot_sg = prot_table-sg_table[prot_page -
- prot_table-page_start_offset];
+ prot_npages = DIV_ROUND_UP(prot_offset + sectors * se_dev-prot_length,
+PAGE_SIZE);
+
+ /* prot pages straddles multiple scatterlist tables */
+ if (prot_table-page_end_offset prot_page + prot_npages - 1) {
+ int i;
+
+ prot_sg = kcalloc(prot_npages, sizeof(*prot_sg), GFP_KERNEL);
+ if (!prot_sg)
+ return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+ need_to_release = true;
+ sg_init_table(prot_sg, prot_npages);
+
+ for (i = 0; i prot_npages; i++) {
+ if (prot_page + i prot_table-page_end_offset) {
+ prot_table = rd_get_prot_table(dev,
+ prot_page + i);
+ if (!prot_table)
+ goto out;
+ sg_unmark_end(prot_sg[i - 1]);
+ }
+ prot_sg[i] = prot_table-sg_table[prot_page + i -
+ prot_table-page_start_offset];
+ }
+ } else {
+ prot_sg = prot_table-sg_table[prot_page -
+ prot_table-page_start_offset];
+ }
Mmm, how about just explicitly doing sg_link() at prot_table scatterlist
creation time instead..?
Yeap, that would be an optimal solution. But some architectures don't
support sg chaining (i.e. !CONFIG_ARCH_HAS_SG_CHAIN).
That would save the extra allocation above, and AFAICT make for simpler
code.
Do you prefer fixing it conditionally with using #ifdef ? (i.e.
sg chaining at creating time if CONFIG_ARCH_HAS_SG_CHAIN is defined,
otherwise do the extra allocation above) If so, I'll resubmit the
patch with such changes.
--
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: smp_processor_id warning in megasas driver on 3.19.3
On Wed, Apr 8, 2015 at 11:17 AM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Wed, 2015-04-08 at 10:59 -0700, Andy Lutomirski wrote: This is a regression somewhere between 3.15 and 3.19.3. Let me know if more diagnostics would be helpful. It's not a regression. Likely someone turned on additional warnings. So the problem is that the warning is incorrect: the use of smp_processor_id() isn't pre-empt unsafe. The driver is using it as a hint as to which queue it should be using, so it doesn't matter if pre-empt schedules the driver thread away from that CPU. The warning goes a long way back. For example, this change from 2005 seems to have refactored it: commit 39c715b71740c4a78ba4769fb54826929bac03cb Author: Ingo Molnar mi...@elte.hu Date: Tue Jun 21 17:14:34 2005 -0700 [PATCH] smp_processor_id() cleanup I presume the warning is because whoever added it thinks that you should be using the get/put cpu API, which would be wholly inappropriate here because we don't want to bind the thread, we just want a hint about the queue. Would raw_smp_processor_id be a good compromise? I'm testing a patch right now and, if it works, I can send it and cc stable. --Andy -- 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: smp_processor_id warning in megasas driver on 3.19.3
On Wed, 2015-04-08 at 11:26 -0700, Andy Lutomirski wrote: On Wed, Apr 8, 2015 at 11:17 AM, James Bottomley james.bottom...@hansenpartnership.com wrote: On Wed, 2015-04-08 at 10:59 -0700, Andy Lutomirski wrote: This is a regression somewhere between 3.15 and 3.19.3. Let me know if more diagnostics would be helpful. It's not a regression. Likely someone turned on additional warnings. So the problem is that the warning is incorrect: the use of smp_processor_id() isn't pre-empt unsafe. The driver is using it as a hint as to which queue it should be using, so it doesn't matter if pre-empt schedules the driver thread away from that CPU. The warning goes a long way back. For example, this change from 2005 seems to have refactored it: commit 39c715b71740c4a78ba4769fb54826929bac03cb Author: Ingo Molnar mi...@elte.hu Date: Tue Jun 21 17:14:34 2005 -0700 [PATCH] smp_processor_id() cleanup I presume the warning is because whoever added it thinks that you should be using the get/put cpu API, which would be wholly inappropriate here because we don't want to bind the thread, we just want a hint about the queue. Would raw_smp_processor_id be a good compromise? I'm testing a patch right now and, if it works, I can send it and cc stable. Anything that doesn't dump the warning would be fine. Of course, the current queue selection smp_processor_id() % instance-msix_vectors Is a bit suboptimal anyway, so perhaps avago would like to fix it more elegantly. James -- 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] target: Fix COMPARE_AND_WRITE with SG_TO_MEM_NOALLOC handling
On Wed, 2015-04-08 at 18:33 +0200, Christoph Hellwig wrote: This fixes the crash that I saw, but the Compare and Write testcast in the scsi testsuite still fails.. I'm not seeing this on my end: root@scsi-mq:/usr/src/libiscsi.git# ./test-tool/iscsi-test-cu --test LINUX.CompareAndWrite /dev/sg0 --dataloss [SKIPPED] REPORT_SUPPORTED_OPCODES is not implemented. CUnit - A Unit testing framework for C - Version 2.1-0 http://cunit.sourceforge.net/ Suite: CompareAndWrite Test: Simple ... passed Test: DpoFua ... [SKIPPED] REPORT_SUPPORTED_OPCODES is not implemented. REPORT_SUPPORTED_OPCODES not implemented. Skipping this part of the test passed Test: Miscompare ... passed --Run Summary: Type Total Ran Passed Failed suites1 1 n/a 0 tests 3 3 3 0 asserts204820482048 0 Tests completed with return value: 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: smp_processor_id warning in megasas driver on 3.19.3
On Wed, Apr 08, 2015 at 10:59:36AM -0700, Andy Lutomirski wrote:
This is a regression somewhere between 3.15 and 3.19.3. Let me know
if more diagnostics would be helpful.
Try this patch:
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 675b5e7..5a0800d 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -1584,11 +1584,11 @@ megasas_build_ldio_fusion(struct megasas_instance
*instance,
fp_possible = io_info.fpOkForIo;
}
- /* Use smp_processor_id() for now until cmd-request-cpu is CPU
+ /* Use raw_smp_processor_id() for now until cmd-request-cpu is CPU
id by default, not CPU group id, otherwise all MSI-X queues won't
be utilized */
cmd-request_desc-SCSIIO.MSIxIndex = instance-msix_vectors ?
- smp_processor_id() % instance-msix_vectors : 0;
+ raw_smp_processor_id() % instance-msix_vectors : 0;
if (fp_possible) {
megasas_set_pd_lba(io_request, scp-cmd_len, io_info, scp,
@@ -1693,7 +1693,10 @@ megasas_build_dcdb_fusion(struct megasas_instance
*instance,
MR_RAID_CTX_RAID_FLAGS_IO_SUB_TYPE_SHIFT;
cmd-request_desc-SCSIIO.DevHandle = io_request-DevHandle;
cmd-request_desc-SCSIIO.MSIxIndex =
- instance-msix_vectors ? smp_processor_id() %
instance-msix_vectors : 0;
+ instance-msix_vectors ?
+ raw_smp_processor_id() %
+ instance-msix_vectors :
+ 0;
os_timeout_value = scmd-request-timeout / HZ;
if (instance-secure_jbod_support
--
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: smp_processor_id warning in megasas driver on 3.19.3
-Original Message-
From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
ow...@vger.kernel.org] On Behalf Of Andy Lutomirski
Sent: Wednesday, April 8, 2015 1:27 PM
To: James Bottomley
Cc: Kashyap Desai; Sumit Saxena; Uday Lingala;
megaraidlinux@avagotech.com; Linux SCSI List
Subject: Re: smp_processor_id warning in megasas driver on 3.19.3
...
Would raw_smp_processor_id be a good compromise? I'm testing a patch
right now and, if it works, I can send it and cc stable.
--Andy
That's what hpsa uses.
---
Robert Elliott, HP Server Storage
N�r��yb�X��ǧv�^�){.n�+{{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj��!�i
Re: smp_processor_id warning in megasas driver on 3.19.3
On Wed, Apr 8, 2015 at 11:51 AM, Christoph Hellwig h...@infradead.org wrote:
On Wed, Apr 08, 2015 at 10:59:36AM -0700, Andy Lutomirski wrote:
This is a regression somewhere between 3.15 and 3.19.3. Let me know
if more diagnostics would be helpful.
Try this patch:
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 675b5e7..5a0800d 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -1584,11 +1584,11 @@ megasas_build_ldio_fusion(struct megasas_instance
*instance,
fp_possible = io_info.fpOkForIo;
}
- /* Use smp_processor_id() for now until cmd-request-cpu is CPU
+ /* Use raw_smp_processor_id() for now until cmd-request-cpu is CPU
id by default, not CPU group id, otherwise all MSI-X queues won't
be utilized */
cmd-request_desc-SCSIIO.MSIxIndex = instance-msix_vectors ?
- smp_processor_id() % instance-msix_vectors : 0;
+ raw_smp_processor_id() % instance-msix_vectors : 0;
if (fp_possible) {
megasas_set_pd_lba(io_request, scp-cmd_len, io_info, scp,
@@ -1693,7 +1693,10 @@ megasas_build_dcdb_fusion(struct megasas_instance
*instance,
MR_RAID_CTX_RAID_FLAGS_IO_SUB_TYPE_SHIFT;
cmd-request_desc-SCSIIO.DevHandle = io_request-DevHandle;
cmd-request_desc-SCSIIO.MSIxIndex =
- instance-msix_vectors ? smp_processor_id() %
instance-msix_vectors : 0;
+ instance-msix_vectors ?
+ raw_smp_processor_id() %
+ instance-msix_vectors :
+ 0;
os_timeout_value = scmd-request-timeout / HZ;
if (instance-secure_jbod_support
I wrote an effectively identical patch (differed in whitespace and
formatting), and it seems to work.
Tested-by: Andy Lutomirski l...@kernel.org
--Andy
--
Andy Lutomirski
AMA Capital Management, LLC
--
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] target: Fix COMPARE_AND_WRITE with SG_TO_MEM_NOALLOC handling
This fixes the crash that I saw, but the Compare and Write testcast in the scsi testsuite still fails.. -- 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] scsi: storvsc: Miscellaneous enhancements and fixes
On Fri, 2015-03-27 at 00:26 -0700, K. Y. Srinivasan wrote: This patch-set addresses perf issues discovered on the Azure storage stack. These patches also fix a couple of bugs. As in the first version of this patch-set, some of the patches are simply a resend. I have bumped up the version number of all patches though. In this version, I have addressed issues raised by Olaf Hering o...@aepfle.de, Long Li lon...@microsoft.com and Venkatesh Srinivas venkate...@google.com Well, I was waiting for a reviewed-by from Olaf and Venkatesh, but I guess that's not forthcoming. James -- 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 01/14] parport: return value of attach and parport_register_driver
On Wed, Apr 08, 2015 at 03:27:37PM +0300, Dan Carpenter wrote: On Wed, Apr 08, 2015 at 05:20:10PM +0530, Sudip Mukherjee wrote: On Wed, Apr 08, 2015 at 02:38:32PM +0300, Dan Carpenter wrote: 1) We can't apply this patch on its own so this way of breaking up the patches doesn't work. yes, if the first patch is reverted for any reason all the others need to be reverted also. so then everything in one single patch? The problem is that patch 1/1 breaks the build. The rule is that we should be able to apply part of a patch series and nothing breaks. If we apply the patch series out of order than things break that's our problem, yes. But if we apply only 1/1 and it breaks, that's a problem with the series. Yep, keep in mind that git bisect will randomly land in the middle of any set of patches during a debugging session and it could very well land on this one. If it breaks, that's a real pain for the people trying to bisect their bug because suddenly they have to deal with a second bug totally different from theirs. Regards, Willy -- 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
smp_processor_id warning in megasas driver on 3.19.3
This is a regression somewhere between 3.15 and 3.19.3. Let me know if more diagnostics would be helpful. [58635.050733] BUG: using smp_processor_id() in preemptible [] code: apt-check/13293 [58635.059861] caller is debug_smp_processor_id+0x17/0x20 [58635.059863] CPU: 0 PID: 13293 Comm: apt-check Not tainted 3.19.3-ama+ #44 [58635.059864] Hardware name: Dell Inc. PowerEdge R620/0KCKR5, BIOS 1.2.6 05/10/2012 [58635.059865] 81810777 88041898 815d6ae5 0002 [58635.059867] 880418c8 81307945 5000 [58635.059869] 880035745e00 88040dc67780 880418d8 [58635.059871] Call Trace: [58635.059874] [815d6ae5] dump_stack+0x4f/0x7b [58635.059877] [81307945] check_preemption_disabled+0xe5/0x100 [58635.059879] [81307977] debug_smp_processor_id+0x17/0x20 [58635.059884] [a003d079] megasas_build_ldio_fusion+0x259/0x450 [megaraid_sas] [58635.059887] [a003d312] megasas_build_io_fusion+0xa2/0x590 [megaraid_sas] [58635.059890] [815de2e2] ? _raw_spin_unlock_irqrestore+0x22/0x40 [58635.059893] [a003d8b6] megasas_build_and_issue_cmd_fusion+0x66/0x$00 [megaraid_sas] [58635.059896] [a00310dc] megasas_queue_command+0x12c/0x170 [megarai$_sas] [58635.059898] [8141d45b] ? scsi_prep_fn+0xdb/0x180 [58635.059901] [8141c2ae] scsi_dispatch_cmd+0xee/0x340 [58635.059902] [8141f019] scsi_request_fn+0x4b9/0x6c0 [58635.059904] [815de215] ? _raw_spin_lock_irqsave+0x25/0x30 [58635.059906] [812c9077] __blk_run_queue+0x37/0x50 [58635.059908] [812ec639] cfq_insert_request+0x2a9/0x600 [58635.059910] [812c618c] __elv_add_request+0x1bc/0x300 [58635.059912] [812ccd28] blk_flush_plug_list+0x1b8/0x210 [58635.059914] [812cd118] blk_finish_plug+0x18/0x50 [58635.059916] [81154543] __do_page_cache_readahead+0x1d3/0x250 [58635.059919] [81154708] ondemand_readahead+0x148/0x2a0 [58635.059921] [8114880c] ? pagecache_get_page+0x2c/0x1d0 [58635.059923] [811549e1] page_cache_sync_readahead+0x31/0x50 [58635.059924] [81149bf9] generic_file_read_iter+0x449/0x630 [58635.059926] [811acede] new_sync_read+0x7e/0xb0 [58635.059928] [811ae0c8] __vfs_read+0x18/0x50 [58635.059930] [811ae186] vfs_read+0x86/0x140 [58635.059932] [811ae289] SyS_read+0x49/0xb0 [58635.059933] [81045dac] ? do_page_fault+0xc/0x10 [58635.059935] [815dea72] system_call_fastpath+0x12/0x17 Thanks, Andy -- 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][v2] blk-plug: don't flush nested plug lists
[ Sending again with a trimmed CC list to just the lists. Jeff - cc lists that large get blocked by mailing lists... ] On Tue, Apr 07, 2015 at 02:55:13PM -0400, Jeff Moyer wrote: The way the on-stack plugging currently works, each nesting level flushes its own list of I/Os. This can be less than optimal (read awful) for certain workloads. For example, consider an application that issues asynchronous O_DIRECT I/Os. It can send down a bunch of I/Os together in a single io_submit call, only to have each of them dispatched individually down in the bowels of the dirct I/O code. The reason is that there are blk_plug-s instantiated both at the upper call site in do_io_submit and down in do_direct_IO. The latter will submit as little as 1 I/O at a time (if you have a small enough I/O size) instead of performing the batching that the plugging infrastructure is supposed to provide. I'm wondering what impact this will have on filesystem metadata IO that needs to be issued immediately. e.g. we are doing writeback, so there is a high level plug in place and we need to page in btree blocks to do extent allocation. We do readahead at this point, but it looks like this change will prevent the readahead from being issued by the unplug in xfs_buf_iosubmit(). So while I can see how this can make your single microbenchmark better (because it's only doing concurrent direct IO to the block device and hence there are no dependencies between individual IOs), I have significant reservations that it's actually a win for filesystem-based workloads where we need direct control of flushing to minimise IO latency due to IO dependencies... Patches like this one: https://lkml.org/lkml/2015/3/20/442 show similar real-world workload improvements to your patchset by being smarter about using high level plugging to enable cross-file merging of IO, but it still relies on the lower layers of plugging to resolve latency bubbles caused by IO dependencies in the filesystems. NOTE TO SUBSYSTEM MAINTAINERS: Before this patch, blk_finish_plug would always flush the plug list. After this patch, this is only the case for the outer-most plug. If you require the plug list to be flushed, you should be calling blk_flush_plug(current). Btrfs and dm maintainers should take a close look at this patch and ensure they get the right behavior in the end. IOWs, you are saying we need to change all our current unplugs to blk_flush_plug(current) to *try* to maintain the same behaviour as we currently have? I say *try*, because no instead of just flushing the readahead IO on the plug, we'll also flush all the queued data writeback IO onthe high level plug. We don't actually want to do that; we only want to submit the readahead and not the bulk IO that will delay the latency sensitive dependent IOs If that is the case, shouldn't you actually be trying to fix the specific plugging problem you've identified (i.e. do_direct_IO() is flushing far too frequently) rather than making a sweeping generalisation that the IO stack plugging infrastructure needs fundamental change? Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- 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
