Skip to content

Commit 5d50b36

Browse files
author
Samuel Ortiz
committed
NFC: Queue pn533 commands
Instead of returning EBUSY when getting a command while another one is running, we queue them. Upon completion of the pending command, the next one is processed. Besides the fact that it simplifies the pn533 locking scheme, it also comes with the nice side effect of fixing the following warning: [ 82.274297] ===================================== [ 82.274297] [ BUG: bad unlock balance detected! ] [ 82.274298] 3.5.0-rc1+ #1 Not tainted [ 82.274299] ------------------------------------- [ 82.274300] kworker/u:1/16 is trying to release lock (&dev->cmd_lock) at: [ 82.274305] [<ffffffff8144f246>] mutex_unlock+0x9/0xb [ 82.274305] but there are no more locks to release! [ 82.274306] [ 82.274306] other info that might help us debug this: [ 82.274306] 2 locks held by kworker/u:1/16: [ 82.274311] #0: (pn533){.+.+..}, at: [<ffffffff8103a67d>] +process_one_work+0x145/0x2e2 [ 82.274314] #1: ((&dev->cmd_work)){+.+...}, at: [<ffffffff8103a67d>] +process_one_work+0x145/0x2e2 [ 82.274314] [ 82.274314] stack backtrace: [ 82.274315] Pid: 16, comm: kworker/u:1 Not tainted 3.5.0-rc1+ #1 [ 82.274315] Call Trace: [ 82.274317] [<ffffffff8144f246>] ? mutex_unlock+0x9/0xb [ 82.274321] [<ffffffff81059841>] print_unlock_inbalance_bug+0xda/0xe4 [ 82.274323] [<ffffffff8105c74c>] lock_release_non_nested+0xb2/0x232 [ 82.274325] [<ffffffff8105a61e>] ? mark_held_locks+0x6d/0x95 [ 82.274326] [<ffffffff8144f246>] ? mutex_unlock+0x9/0xb [ 82.274328] [<ffffffff81451105>] ? _raw_spin_unlock_irqrestore+0x40/0x5c [ 82.274329] [<ffffffff8144f246>] ? mutex_unlock+0x9/0xb [ 82.274330] [<ffffffff8105ca42>] lock_release+0x176/0x1ac [ 82.274333] [<ffffffff8123de14>] ? pn533_send_complete+0xa8/0xa8 [ 82.274334] [<ffffffff8144f1d6>] __mutex_unlock_slowpath+0xb0/0x117 [ 82.274336] [<ffffffff8144f246>] mutex_unlock+0x9/0xb [ 82.274337] [<ffffffff8123de65>] pn533_wq_cmd_complete+0x51/0x55 [ 82.274338] [<ffffffff8103a6db>] process_one_work+0x1a3/0x2e2 [ 82.274340] [<ffffffff8103a67d>] ? process_one_work+0x145/0x2e2 [ 82.274341] [<ffffffff8103b119>] worker_thread+0xcf/0x153 [ 82.274343] [<ffffffff8103b04a>] ? manage_workers.isra.22+0x16b/0x16b [ 82.274344] [<ffffffff8103b04a>] ? manage_workers.isra.22+0x16b/0x16b [ 82.274346] [<ffffffff8103eb11>] kthread+0x95/0x9d [ 82.274348] [<ffffffff81452ef4>] kernel_thread_helper+0x4/0x10 [ 82.274351] [<ffffffff81046561>] ? finish_task_switch+0x45/0xc3 [ 82.274352] [<ffffffff814514f0>] ? retint_restore_args+0x13/0x13 [ 82.274353] [<ffffffff8103ea7c>] ? __init_kthread_worker+0x55/0x55 [ 82.274354] [<ffffffff81452ef0>] ? gs_change+0x13/0x13 Signed-off-by: Samuel Ortiz <[email protected]>
1 parent 90e6274 commit 5d50b36

File tree

1 file changed

+85
-17
lines changed

1 file changed

+85
-17
lines changed

‎drivers/nfc/pn533.c‎

Lines changed: 85 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,7 @@ struct pn533 {
356356

357357
struct workqueue_struct *wq;
358358
struct work_struct cmd_work;
359+
struct work_struct cmd_complete_work;
359360
struct work_struct poll_work;
360361
struct work_struct mi_work;
361362
struct work_struct tg_work;
@@ -383,6 +384,19 @@ struct pn533 {
383384
u8 tgt_mode;
384385

385386
u32 device_type;
387+
388+
struct list_head cmd_queue;
389+
u8 cmd_pending;
390+
};
391+
392+
struct pn533_cmd {
393+
struct list_head queue;
394+
struct pn533_frame *out_frame;
395+
struct pn533_frame *in_frame;
396+
int in_frame_len;
397+
pn533_cmd_complete_t cmd_complete;
398+
void *arg;
399+
gfp_t flags;
386400
};
387401

388402
struct pn533_frame {
@@ -487,7 +501,7 @@ static bool pn533_rx_frame_is_cmd_response(struct pn533_frame *frame, u8 cmd)
487501

488502
static void pn533_wq_cmd_complete(struct work_struct *work)
489503
{
490-
struct pn533 *dev = container_of(work, struct pn533, cmd_work);
504+
struct pn533 *dev = container_of(work, struct pn533, cmd_complete_work);
491505
struct pn533_frame *in_frame;
492506
int rc;
493507

@@ -502,7 +516,7 @@ static void pn533_wq_cmd_complete(struct work_struct *work)
502516
PN533_FRAME_CMD_PARAMS_LEN(in_frame));
503517

504518
if (rc != -EINPROGRESS)
505-
mutex_unlock(&dev->cmd_lock);
519+
queue_work(dev->wq, &dev->cmd_work);
506520
}
507521

508522
static void pn533_recv_response(struct urb *urb)
@@ -550,7 +564,7 @@ static void pn533_recv_response(struct urb *urb)
550564
dev->wq_in_frame = in_frame;
551565

552566
sched_wq:
553-
queue_work(dev->wq, &dev->cmd_work);
567+
queue_work(dev->wq, &dev->cmd_complete_work);
554568
}
555569

556570
static int pn533_submit_urb_for_response(struct pn533 *dev, gfp_t flags)
@@ -606,7 +620,7 @@ static void pn533_recv_ack(struct urb *urb)
606620

607621
sched_wq:
608622
dev->wq_in_frame = NULL;
609-
queue_work(dev->wq, &dev->cmd_work);
623+
queue_work(dev->wq, &dev->cmd_complete_work);
610624
}
611625

612626
static int pn533_submit_urb_for_ack(struct pn533 *dev, gfp_t flags)
@@ -669,29 +683,76 @@ static int __pn533_send_cmd_frame_async(struct pn533 *dev,
669683
return rc;
670684
}
671685

686+
static void pn533_wq_cmd(struct work_struct *work)
687+
{
688+
struct pn533 *dev = container_of(work, struct pn533, cmd_work);
689+
struct pn533_cmd *cmd;
690+
691+
mutex_lock(&dev->cmd_lock);
692+
693+
if (list_empty(&dev->cmd_queue)) {
694+
dev->cmd_pending = 0;
695+
mutex_unlock(&dev->cmd_lock);
696+
return;
697+
}
698+
699+
cmd = list_first_entry(&dev->cmd_queue, struct pn533_cmd, queue);
700+
701+
mutex_unlock(&dev->cmd_lock);
702+
703+
__pn533_send_cmd_frame_async(dev, cmd->out_frame, cmd->in_frame,
704+
cmd->in_frame_len, cmd->cmd_complete,
705+
cmd->arg, cmd->flags);
706+
707+
list_del(&cmd->queue);
708+
kfree(cmd);
709+
}
710+
672711
static int pn533_send_cmd_frame_async(struct pn533 *dev,
673712
struct pn533_frame *out_frame,
674713
struct pn533_frame *in_frame,
675714
int in_frame_len,
676715
pn533_cmd_complete_t cmd_complete,
677716
void *arg, gfp_t flags)
678717
{
718+
struct pn533_cmd *cmd;
679719
int rc;
680720

681721
nfc_dev_dbg(&dev->interface->dev, "%s", __func__);
682722

683-
if (!mutex_trylock(&dev->cmd_lock))
684-
return -EBUSY;
723+
mutex_lock(&dev->cmd_lock);
685724

686-
rc = __pn533_send_cmd_frame_async(dev, out_frame, in_frame,
687-
in_frame_len, cmd_complete, arg, flags);
688-
if (rc)
689-
goto error;
725+
if (!dev->cmd_pending) {
726+
rc = __pn533_send_cmd_frame_async(dev, out_frame, in_frame,
727+
in_frame_len, cmd_complete,
728+
arg, flags);
729+
if (!rc)
730+
dev->cmd_pending = 1;
731+
732+
mutex_unlock(&dev->cmd_lock);
733+
734+
return rc;
735+
}
736+
737+
nfc_dev_dbg(&dev->interface->dev, "%s Queueing command", __func__);
738+
739+
cmd = kzalloc(sizeof(struct pn533_cmd), flags);
740+
if (!cmd)
741+
return -ENOMEM;
742+
743+
INIT_LIST_HEAD(&cmd->queue);
744+
cmd->out_frame = out_frame;
745+
cmd->in_frame = in_frame;
746+
cmd->in_frame_len = in_frame_len;
747+
cmd->cmd_complete = cmd_complete;
748+
cmd->arg = arg;
749+
cmd->flags = flags;
750+
751+
list_add_tail(&cmd->queue, &dev->cmd_queue);
690752

691-
return 0;
692-
error:
693753
mutex_unlock(&dev->cmd_lock);
694-
return rc;
754+
755+
return 0;
695756
}
696757

697758
struct pn533_sync_cmd_response {
@@ -1305,8 +1366,6 @@ static void pn533_listen_mode_timer(unsigned long data)
13051366

13061367
dev->cancel_listen = 1;
13071368

1308-
mutex_unlock(&dev->cmd_lock);
1309-
13101369
pn533_poll_next_mod(dev);
13111370

13121371
queue_work(dev->wq, &dev->poll_work);
@@ -2131,7 +2190,7 @@ static void pn533_wq_mi_recv(struct work_struct *work)
21312190

21322191
kfree(arg);
21332192

2134-
mutex_unlock(&dev->cmd_lock);
2193+
queue_work(dev->wq, &dev->cmd_work);
21352194
}
21362195

21372196
static int pn533_set_configuration(struct pn533 *dev, u8 cfgitem, u8 *cfgdata,
@@ -2330,7 +2389,8 @@ static int pn533_probe(struct usb_interface *interface,
23302389
NULL, 0,
23312390
pn533_send_complete, dev);
23322391

2333-
INIT_WORK(&dev->cmd_work, pn533_wq_cmd_complete);
2392+
INIT_WORK(&dev->cmd_work, pn533_wq_cmd);
2393+
INIT_WORK(&dev->cmd_complete_work, pn533_wq_cmd_complete);
23342394
INIT_WORK(&dev->mi_work, pn533_wq_mi_recv);
23352395
INIT_WORK(&dev->tg_work, pn533_wq_tg_get_data);
23362396
INIT_WORK(&dev->poll_work, pn533_wq_poll);
@@ -2346,6 +2406,8 @@ static int pn533_probe(struct usb_interface *interface,
23462406

23472407
skb_queue_head_init(&dev->resp_q);
23482408

2409+
INIT_LIST_HEAD(&dev->cmd_queue);
2410+
23492411
usb_set_intfdata(interface, dev);
23502412

23512413
pn533_tx_frame_init(dev->out_frame, PN533_CMD_GET_FIRMWARE_VERSION);
@@ -2417,6 +2479,7 @@ static int pn533_probe(struct usb_interface *interface,
24172479
static void pn533_disconnect(struct usb_interface *interface)
24182480
{
24192481
struct pn533 *dev;
2482+
struct pn533_cmd *cmd, *n;
24202483

24212484
dev = usb_get_intfdata(interface);
24222485
usb_set_intfdata(interface, NULL);
@@ -2433,6 +2496,11 @@ static void pn533_disconnect(struct usb_interface *interface)
24332496

24342497
del_timer(&dev->listen_timer);
24352498

2499+
list_for_each_entry_safe(cmd, n, &dev->cmd_queue, queue) {
2500+
list_del(&cmd->queue);
2501+
kfree(cmd);
2502+
}
2503+
24362504
kfree(dev->in_frame);
24372505
usb_free_urb(dev->in_urb);
24382506
kfree(dev->out_frame);

0 commit comments

Comments
 (0)