From 837c5220557270e652d89f68a9fb12a5e72e8a7a Mon Sep 17 00:00:00 2001 From: Sebastian Ott Date: Tue, 19 Jul 2016 10:43:26 +0200 Subject: s390/cio: convert cfg_lock mutex to spinlock cfg_lock is never held long and we don't want to sleep while the lock is being held. Thus it can be converted to a simple spinlock. In addition we can now use the lock during the evaluation of a wake_up condition. Signed-off-by: Sebastian Ott Reviewed-by: Peter Oberparleiter Signed-off-by: Martin Schwidefsky --- drivers/s390/cio/chp.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'drivers/s390') diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c index e96aced58627..c602211ab94e 100644 --- a/drivers/s390/cio/chp.c +++ b/drivers/s390/cio/chp.c @@ -37,7 +37,7 @@ enum cfg_task_t { /* Map for pending configure tasks. */ static enum cfg_task_t chp_cfg_task[__MAX_CSSID + 1][__MAX_CHPID + 1]; -static DEFINE_MUTEX(cfg_lock); +static DEFINE_SPINLOCK(cfg_lock); static int cfg_busy; /* Map for channel-path status. */ @@ -674,7 +674,7 @@ static void cfg_func(struct work_struct *work) enum cfg_task_t t; int rc; - mutex_lock(&cfg_lock); + spin_lock(&cfg_lock); t = cfg_none; chp_id_for_each(&chpid) { t = cfg_get_task(chpid); @@ -683,7 +683,7 @@ static void cfg_func(struct work_struct *work) break; } } - mutex_unlock(&cfg_lock); + spin_unlock(&cfg_lock); switch (t) { case cfg_configure: @@ -709,9 +709,9 @@ static void cfg_func(struct work_struct *work) case cfg_none: /* Get updated information after last change. */ info_update(); - mutex_lock(&cfg_lock); + spin_lock(&cfg_lock); cfg_busy = 0; - mutex_unlock(&cfg_lock); + spin_unlock(&cfg_lock); wake_up_interruptible(&cfg_wait_queue); return; } @@ -729,10 +729,10 @@ void chp_cfg_schedule(struct chp_id chpid, int configure) { CIO_MSG_EVENT(2, "chp_cfg_sched%x.%02x=%d\n", chpid.cssid, chpid.id, configure); - mutex_lock(&cfg_lock); + spin_lock(&cfg_lock); cfg_set_task(chpid, configure ? cfg_configure : cfg_deconfigure); cfg_busy = 1; - mutex_unlock(&cfg_lock); + spin_unlock(&cfg_lock); schedule_work(&cfg_work); } @@ -746,10 +746,10 @@ void chp_cfg_schedule(struct chp_id chpid, int configure) void chp_cfg_cancel_deconfigure(struct chp_id chpid) { CIO_MSG_EVENT(2, "chp_cfg_cancel:%x.%02x\n", chpid.cssid, chpid.id); - mutex_lock(&cfg_lock); + spin_lock(&cfg_lock); if (cfg_get_task(chpid) == cfg_deconfigure) cfg_set_task(chpid, cfg_none); - mutex_unlock(&cfg_lock); + spin_unlock(&cfg_lock); } static int cfg_wait_idle(void) -- cgit v1.2.3 From 4475aeb8b77db34dea96b09900ba0cb245b6fb42 Mon Sep 17 00:00:00 2001 From: Sebastian Ott Date: Tue, 19 Jul 2016 10:53:35 +0200 Subject: s390/cio: fix premature wakeup during chp configure We store requests for channel path configure operations in an array but maintain an additional cfg_busy variable (indicating if we have requests stored in said array). When 2 tasks request a channel path configure operation cfg_busy could be set to 0 even if we still have unprocessed requests. This would lead to the second task being woken up although its request was not processed yet. Fix that by getting rid of cfg_busy and use the chp_cfg_task array in the wake up condition. Signed-off-by: Sebastian Ott Reviewed-by: Peter Oberparleiter Signed-off-by: Martin Schwidefsky --- drivers/s390/cio/chp.c | 46 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 14 deletions(-) (limited to 'drivers/s390') diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c index c602211ab94e..46be25c7461e 100644 --- a/drivers/s390/cio/chp.c +++ b/drivers/s390/cio/chp.c @@ -38,7 +38,6 @@ enum cfg_task_t { /* Map for pending configure tasks. */ static enum cfg_task_t chp_cfg_task[__MAX_CSSID + 1][__MAX_CHPID + 1]; static DEFINE_SPINLOCK(cfg_lock); -static int cfg_busy; /* Map for channel-path status. */ static struct sclp_chp_info chp_info; @@ -666,6 +665,20 @@ static void cfg_set_task(struct chp_id chpid, enum cfg_task_t cfg) chp_cfg_task[chpid.cssid][chpid.id] = cfg; } +/* Fetch the first configure task. Set chpid accordingly. */ +static enum cfg_task_t chp_cfg_fetch_task(struct chp_id *chpid) +{ + enum cfg_task_t t = cfg_none; + + chp_id_for_each(chpid) { + t = cfg_get_task(*chpid); + if (t != cfg_none) + break; + } + + return t; +} + /* Perform one configure/deconfigure request. Reschedule work function until * last request. */ static void cfg_func(struct work_struct *work) @@ -675,14 +688,7 @@ static void cfg_func(struct work_struct *work) int rc; spin_lock(&cfg_lock); - t = cfg_none; - chp_id_for_each(&chpid) { - t = cfg_get_task(chpid); - if (t != cfg_none) { - cfg_set_task(chpid, cfg_none); - break; - } - } + t = chp_cfg_fetch_task(&chpid); spin_unlock(&cfg_lock); switch (t) { @@ -709,12 +715,13 @@ static void cfg_func(struct work_struct *work) case cfg_none: /* Get updated information after last change. */ info_update(); - spin_lock(&cfg_lock); - cfg_busy = 0; - spin_unlock(&cfg_lock); wake_up_interruptible(&cfg_wait_queue); return; } + spin_lock(&cfg_lock); + if (t == cfg_get_task(chpid)) + cfg_set_task(chpid, cfg_none); + spin_unlock(&cfg_lock); schedule_work(&cfg_work); } @@ -731,7 +738,6 @@ void chp_cfg_schedule(struct chp_id chpid, int configure) configure); spin_lock(&cfg_lock); cfg_set_task(chpid, configure ? cfg_configure : cfg_deconfigure); - cfg_busy = 1; spin_unlock(&cfg_lock); schedule_work(&cfg_work); } @@ -752,9 +758,21 @@ void chp_cfg_cancel_deconfigure(struct chp_id chpid) spin_unlock(&cfg_lock); } +static bool cfg_idle(void) +{ + struct chp_id chpid; + enum cfg_task_t t; + + spin_lock(&cfg_lock); + t = chp_cfg_fetch_task(&chpid); + spin_unlock(&cfg_lock); + + return t == cfg_none; +} + static int cfg_wait_idle(void) { - if (wait_event_interruptible(cfg_wait_queue, !cfg_busy)) + if (wait_event_interruptible(cfg_wait_queue, cfg_idle())) return -ERESTARTSYS; return 0; } -- cgit v1.2.3 From d6d86c57d77d466df2096b134e5f54463d3f0fb8 Mon Sep 17 00:00:00 2001 From: Ingo Tuchscherer Date: Mon, 25 Jul 2016 14:52:28 +0200 Subject: s390/zcrypt: Fix zcrypt suspend/resume behavior The device suspend call triggers all ap devices to fetch potentially available response messages from the queues. Therefore the corresponding zcrypt device, that is allocated asynchronously after ap device probing, needs to be fully prepared. This race condition could lead to uninitialized response buffers while trying to read from the queues. Introduce a new callback within the ap layer to get noticed when a zcrypt device is fully prepared. Additional checks prevent reading from devices that are not fully prepared. Signed-off-by: Ingo Tuchscherer Signed-off-by: Martin Schwidefsky --- drivers/s390/crypto/ap_bus.c | 46 ++++++++++++++++++++++++++++++++++--- drivers/s390/crypto/ap_bus.h | 1 + drivers/s390/crypto/zcrypt_cex2a.c | 2 +- drivers/s390/crypto/zcrypt_cex4.c | 2 +- drivers/s390/crypto/zcrypt_pcixcc.c | 2 +- 5 files changed, 47 insertions(+), 6 deletions(-) (limited to 'drivers/s390') diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c index 4feb27215ab6..03e4d6246d87 100644 --- a/drivers/s390/crypto/ap_bus.c +++ b/drivers/s390/crypto/ap_bus.c @@ -468,6 +468,8 @@ int ap_recv(ap_qid_t qid, unsigned long long *psmid, void *msg, size_t length) { struct ap_queue_status status; + if (msg == NULL) + return -EINVAL; status = __ap_recv(qid, psmid, msg, length); switch (status.response_code) { case AP_RESPONSE_NORMAL: @@ -617,6 +619,8 @@ static enum ap_wait ap_sm_read(struct ap_device *ap_dev) { struct ap_queue_status status; + if (!ap_dev->reply) + return AP_WAIT_NONE; status = ap_sm_recv(ap_dev); switch (status.response_code) { case AP_RESPONSE_NORMAL: @@ -637,6 +641,31 @@ static enum ap_wait ap_sm_read(struct ap_device *ap_dev) } } +/** + * ap_sm_suspend_read(): Receive pending reply messages from an AP device + * without changing the device state in between. In suspend mode we don't + * allow sending new requests, therefore just fetch pending replies. + * @ap_dev: pointer to the AP device + * + * Returns AP_WAIT_NONE or AP_WAIT_AGAIN + */ +static enum ap_wait ap_sm_suspend_read(struct ap_device *ap_dev) +{ + struct ap_queue_status status; + + if (!ap_dev->reply) + return AP_WAIT_NONE; + status = ap_sm_recv(ap_dev); + switch (status.response_code) { + case AP_RESPONSE_NORMAL: + if (ap_dev->queue_count > 0) + return AP_WAIT_AGAIN; + /* fall through */ + default: + return AP_WAIT_NONE; + } +} + /** * ap_sm_write(): Send messages from the request queue to an AP device. * @ap_dev: pointer to the AP device @@ -738,7 +767,7 @@ static enum ap_wait ap_sm_reset_wait(struct ap_device *ap_dev) struct ap_queue_status status; unsigned long info; - if (ap_dev->queue_count > 0) + if (ap_dev->queue_count > 0 && ap_dev->reply) /* Try to read a completed message and get the status */ status = ap_sm_recv(ap_dev); else @@ -778,7 +807,7 @@ static enum ap_wait ap_sm_setirq_wait(struct ap_device *ap_dev) struct ap_queue_status status; unsigned long info; - if (ap_dev->queue_count > 0) + if (ap_dev->queue_count > 0 && ap_dev->reply) /* Try to read a completed message and get the status */ status = ap_sm_recv(ap_dev); else @@ -834,7 +863,7 @@ static ap_func_t *ap_jumptable[NR_AP_STATES][NR_AP_EVENTS] = { [AP_EVENT_TIMEOUT] = ap_sm_reset, }, [AP_STATE_SUSPEND_WAIT] = { - [AP_EVENT_POLL] = ap_sm_read, + [AP_EVENT_POLL] = ap_sm_suspend_read, [AP_EVENT_TIMEOUT] = ap_sm_nop, }, [AP_STATE_BORKED] = { @@ -1335,6 +1364,17 @@ static struct bus_type ap_bus_type = { .resume = ap_dev_resume, }; +void ap_device_init_reply(struct ap_device *ap_dev, + struct ap_message *reply) +{ + ap_dev->reply = reply; + + spin_lock_bh(&ap_dev->lock); + ap_sm_wait(ap_sm_event(ap_dev, AP_EVENT_POLL)); + spin_unlock_bh(&ap_dev->lock); +} +EXPORT_SYMBOL(ap_device_init_reply); + static int ap_device_probe(struct device *dev) { struct ap_device *ap_dev = to_ap_dev(dev); diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h index 6adcbdf225d1..d7fdf5c024d7 100644 --- a/drivers/s390/crypto/ap_bus.h +++ b/drivers/s390/crypto/ap_bus.h @@ -262,6 +262,7 @@ void ap_queue_message(struct ap_device *ap_dev, struct ap_message *ap_msg); void ap_cancel_message(struct ap_device *ap_dev, struct ap_message *ap_msg); void ap_flush_queue(struct ap_device *ap_dev); void ap_bus_force_rescan(void); +void ap_device_init_reply(struct ap_device *ap_dev, struct ap_message *ap_msg); int ap_module_init(void); void ap_module_exit(void); diff --git a/drivers/s390/crypto/zcrypt_cex2a.c b/drivers/s390/crypto/zcrypt_cex2a.c index 1e849d6e1dfe..15104aaa075a 100644 --- a/drivers/s390/crypto/zcrypt_cex2a.c +++ b/drivers/s390/crypto/zcrypt_cex2a.c @@ -126,7 +126,7 @@ static int zcrypt_cex2a_probe(struct ap_device *ap_dev) MSGTYPE50_VARIANT_DEFAULT); zdev->ap_dev = ap_dev; zdev->online = 1; - ap_dev->reply = &zdev->reply; + ap_device_init_reply(ap_dev, &zdev->reply); ap_dev->private = zdev; rc = zcrypt_device_register(zdev); if (rc) { diff --git a/drivers/s390/crypto/zcrypt_cex4.c b/drivers/s390/crypto/zcrypt_cex4.c index bb3908818505..ccb2e78ebf0e 100644 --- a/drivers/s390/crypto/zcrypt_cex4.c +++ b/drivers/s390/crypto/zcrypt_cex4.c @@ -147,7 +147,7 @@ static int zcrypt_cex4_probe(struct ap_device *ap_dev) return -ENODEV; zdev->ap_dev = ap_dev; zdev->online = 1; - ap_dev->reply = &zdev->reply; + ap_device_init_reply(ap_dev, &zdev->reply); ap_dev->private = zdev; rc = zcrypt_device_register(zdev); if (rc) { diff --git a/drivers/s390/crypto/zcrypt_pcixcc.c b/drivers/s390/crypto/zcrypt_pcixcc.c index f41852768953..df8f0c4dacb7 100644 --- a/drivers/s390/crypto/zcrypt_pcixcc.c +++ b/drivers/s390/crypto/zcrypt_pcixcc.c @@ -327,7 +327,7 @@ static int zcrypt_pcixcc_probe(struct ap_device *ap_dev) else zdev->ops = zcrypt_msgtype_request(MSGTYPE06_NAME, MSGTYPE06_VARIANT_NORNG); - ap_dev->reply = &zdev->reply; + ap_device_init_reply(ap_dev, &zdev->reply); ap_dev->private = zdev; rc = zcrypt_device_register(zdev); if (rc) -- cgit v1.2.3 From 33c388b81ce4b2f249730014b9b9f103a7578ca2 Mon Sep 17 00:00:00 2001 From: Wei Yongjun Date: Sun, 31 Jul 2016 11:47:06 +0200 Subject: s390/zcrypt: fix possible memory leak in ap_module_init() ap_configuration is malloced in ap_module_init() and should be freed before leaving from the error handling cases, otherwise it may cause memory leak. Signed-off-by: Wei Yongjun Signed-off-by: Martin Schwidefsky --- drivers/s390/crypto/ap_bus.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'drivers/s390') diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c index 03e4d6246d87..ed92fb09fc8e 100644 --- a/drivers/s390/crypto/ap_bus.c +++ b/drivers/s390/crypto/ap_bus.c @@ -1819,7 +1819,8 @@ int __init ap_module_init(void) if (ap_domain_index < -1 || ap_domain_index > max_domain_id) { pr_warn("%d is not a valid cryptographic domain\n", ap_domain_index); - return -EINVAL; + rc = -EINVAL; + goto out_free; } /* In resume callback we need to know if the user had set the domain. * If so, we can not just reset it. @@ -1892,6 +1893,7 @@ out: unregister_reset_call(&ap_reset_call); if (ap_using_interrupts()) unregister_adapter_interrupt(&ap_airq); +out_free: kfree(ap_configuration); return rc; } -- cgit v1.2.3