]>
Commit | Line | Data |
---|---|---|
6a930a95 BS |
1 | From: Vasu Dev <vasu.dev@intel.com> |
2 | Subject: libfc, fcoe: fixed locking issues with lport->lp_mutex around lport->link_status | |
3 | Patch-mainline: 6d235742e63f6b8912d8b200b75f9aa6d48f3e07 | |
4 | References: bnc #468053 | |
5 | ||
6 | The fcoe_xmit could call fc_pause in case the pending skb queue len is larger | |
7 | than FCOE_MAX_QUEUE_DEPTH, the fc_pause was trying to grab lport->lp_muex to | |
8 | change lport->link_status and that had these issues :- | |
9 | ||
10 | 1. The fcoe_xmit was getting called with bh disabled, thus causing | |
11 | "BUG: scheduling while atomic" when grabbing lport->lp_muex with bh disabled. | |
12 | ||
13 | 2. fc_linkup and fc_linkdown function calls lport_enter function with | |
14 | lport->lp_mutex held and these enter function in turn calls fcoe_xmit to send | |
15 | lport related FC frame, e.g. fc_linkup => fc_lport_enter_flogi to send flogi | |
16 | req. In this case grabbing the same lport->lp_mutex again in fc_puase from | |
17 | fcoe_xmit would cause deadlock. | |
18 | ||
19 | The lport->lp_mutex was used for setting FC_PAUSE in fcoe_xmit path but | |
20 | FC_PAUSE bit was not used anywhere beside just setting and clear this | |
21 | bit in lport->link_status, instead used a separate field qfull in fc_lport | |
22 | to eliminate need for lport->lp_mutex to track pending queue full condition | |
23 | and in turn avoid above described two locking issues. | |
24 | ||
25 | Also added check for lp->qfull in fc_fcp_lport_queue_ready to trigger | |
26 | SCSI_MLQUEUE_HOST_BUSY when lp->qfull is set to prevent more scsi-ml cmds | |
27 | while lp->qfull is set. | |
28 | ||
29 | This patch eliminated FC_LINK_UP and FC_PAUSE and instead used dedicated | |
30 | fields in fc_lport for this, this simplified all related conditional | |
31 | code. | |
32 | ||
33 | Also removed fc_pause and fc_unpause functions and instead used newly added | |
34 | lport->qfull directly in fcoe. | |
35 | ||
36 | Also fixed a circular locking in fc_exch_recv_abts. | |
37 | ||
38 | These issues were blocking large file copy to a 2TB lun. | |
39 | ||
40 | Signed-off-by: Vasu Dev <vasu.dev@intel.com> | |
41 | Acked-by: Bernhard Walle <bwalle@suse.de> | |
42 | --- | |
43 | drivers/scsi/fcoe/fcoe_sw.c | 6 +++--- | |
44 | drivers/scsi/fcoe/libfcoe.c | 41 +++++++++++++++++------------------------ | |
45 | drivers/scsi/libfc/fc_exch.c | 2 +- | |
46 | drivers/scsi/libfc/fc_fcp.c | 6 +++--- | |
47 | drivers/scsi/libfc/fc_lport.c | 38 +++++++------------------------------- | |
48 | drivers/scsi/libfc/fc_rport.c | 2 +- | |
49 | include/scsi/libfc.h | 12 ++---------- | |
50 | 7 files changed, 34 insertions(+), 73 deletions(-) | |
51 | ||
52 | --- a/drivers/scsi/fcoe/fcoe_sw.c | |
53 | +++ b/drivers/scsi/fcoe/fcoe_sw.c | |
54 | @@ -116,7 +116,8 @@ static int fcoe_sw_lport_config(struct f | |
55 | { | |
56 | int i = 0; | |
57 | ||
58 | - lp->link_status = 0; | |
59 | + lp->link_up = 0; | |
60 | + lp->qfull = 0; | |
61 | lp->max_retry_count = 3; | |
62 | lp->e_d_tov = 2 * 1000; /* FC-FS default */ | |
63 | lp->r_a_tov = 2 * 2 * 1000; | |
64 | @@ -181,9 +182,8 @@ static int fcoe_sw_netdev_config(struct | |
65 | if (fc_set_mfs(lp, mfs)) | |
66 | return -EINVAL; | |
67 | ||
68 | - lp->link_status = ~FC_PAUSE & ~FC_LINK_UP; | |
69 | if (!fcoe_link_ok(lp)) | |
70 | - lp->link_status |= FC_LINK_UP; | |
71 | + lp->link_up = 1; | |
72 | ||
73 | /* offload features support */ | |
74 | if (fc->real_dev->features & NETIF_F_SG) | |
75 | --- a/drivers/scsi/fcoe/libfcoe.c | |
76 | +++ b/drivers/scsi/fcoe/libfcoe.c | |
77 | @@ -505,7 +505,7 @@ int fcoe_xmit(struct fc_lport *lp, struc | |
78 | if (rc) { | |
79 | fcoe_insert_wait_queue(lp, skb); | |
80 | if (fc->fcoe_pending_queue.qlen > FCOE_MAX_QUEUE_DEPTH) | |
81 | - fc_pause(lp); | |
82 | + lp->qfull = 1; | |
83 | } | |
84 | ||
85 | return 0; | |
86 | @@ -719,7 +719,7 @@ static void fcoe_recv_flogi(struct fcoe_ | |
87 | * fcoe_watchdog - fcoe timer callback | |
88 | * @vp: | |
89 | * | |
90 | - * This checks the pending queue length for fcoe and put fcoe to be paused state | |
91 | + * This checks the pending queue length for fcoe and set lport qfull | |
92 | * if the FCOE_MAX_QUEUE_DEPTH is reached. This is done for all fc_lport on the | |
93 | * fcoe_hostlist. | |
94 | * | |
95 | @@ -729,17 +729,17 @@ void fcoe_watchdog(ulong vp) | |
96 | { | |
97 | struct fc_lport *lp; | |
98 | struct fcoe_softc *fc; | |
99 | - int paused = 0; | |
100 | + int qfilled = 0; | |
101 | ||
102 | read_lock(&fcoe_hostlist_lock); | |
103 | list_for_each_entry(fc, &fcoe_hostlist, list) { | |
104 | lp = fc->lp; | |
105 | if (lp) { | |
106 | if (fc->fcoe_pending_queue.qlen > FCOE_MAX_QUEUE_DEPTH) | |
107 | - paused = 1; | |
108 | + qfilled = 1; | |
109 | if (fcoe_check_wait_queue(lp) < FCOE_MAX_QUEUE_DEPTH) { | |
110 | - if (paused) | |
111 | - fc_unpause(lp); | |
112 | + if (qfilled) | |
113 | + lp->qfull = 0; | |
114 | } | |
115 | } | |
116 | } | |
117 | @@ -768,8 +768,7 @@ void fcoe_watchdog(ulong vp) | |
118 | **/ | |
119 | static int fcoe_check_wait_queue(struct fc_lport *lp) | |
120 | { | |
121 | - int rc, unpause = 0; | |
122 | - int paused = 0; | |
123 | + int rc; | |
124 | struct sk_buff *skb; | |
125 | struct fcoe_softc *fc; | |
126 | ||
127 | @@ -777,10 +776,10 @@ static int fcoe_check_wait_queue(struct | |
128 | spin_lock_bh(&fc->fcoe_pending_queue.lock); | |
129 | ||
130 | /* | |
131 | - * is this interface paused? | |
132 | + * if interface pending queue full then set qfull in lport. | |
133 | */ | |
134 | if (fc->fcoe_pending_queue.qlen > FCOE_MAX_QUEUE_DEPTH) | |
135 | - paused = 1; | |
136 | + lp->qfull = 1; | |
137 | if (fc->fcoe_pending_queue.qlen) { | |
138 | while ((skb = __skb_dequeue(&fc->fcoe_pending_queue)) != NULL) { | |
139 | spin_unlock_bh(&fc->fcoe_pending_queue.lock); | |
140 | @@ -792,11 +791,9 @@ static int fcoe_check_wait_queue(struct | |
141 | spin_lock_bh(&fc->fcoe_pending_queue.lock); | |
142 | } | |
143 | if (fc->fcoe_pending_queue.qlen < FCOE_MAX_QUEUE_DEPTH) | |
144 | - unpause = 1; | |
145 | + lp->qfull = 0; | |
146 | } | |
147 | spin_unlock_bh(&fc->fcoe_pending_queue.lock); | |
148 | - if ((unpause) && (paused)) | |
149 | - fc_unpause(lp); | |
150 | return fc->fcoe_pending_queue.qlen; | |
151 | } | |
152 | ||
153 | @@ -874,7 +871,7 @@ static int fcoe_device_notification(stru | |
154 | struct net_device *real_dev = ptr; | |
155 | struct fcoe_softc *fc; | |
156 | struct fcoe_dev_stats *stats; | |
157 | - u16 new_status; | |
158 | + u32 new_link_up; | |
159 | u32 mfs; | |
160 | int rc = NOTIFY_OK; | |
161 | ||
162 | @@ -891,17 +888,15 @@ static int fcoe_device_notification(stru | |
163 | goto out; | |
164 | } | |
165 | ||
166 | - new_status = lp->link_status; | |
167 | + new_link_up = lp->link_up; | |
168 | switch (event) { | |
169 | case NETDEV_DOWN: | |
170 | case NETDEV_GOING_DOWN: | |
171 | - new_status &= ~FC_LINK_UP; | |
172 | + new_link_up = 0; | |
173 | break; | |
174 | case NETDEV_UP: | |
175 | case NETDEV_CHANGE: | |
176 | - new_status &= ~FC_LINK_UP; | |
177 | - if (!fcoe_link_ok(lp)) | |
178 | - new_status |= FC_LINK_UP; | |
179 | + new_link_up = !fcoe_link_ok(lp); | |
180 | break; | |
181 | case NETDEV_CHANGEMTU: | |
182 | mfs = fc->real_dev->mtu - | |
183 | @@ -909,17 +904,15 @@ static int fcoe_device_notification(stru | |
184 | sizeof(struct fcoe_crc_eof)); | |
185 | if (mfs >= FC_MIN_MAX_FRAME) | |
186 | fc_set_mfs(lp, mfs); | |
187 | - new_status &= ~FC_LINK_UP; | |
188 | - if (!fcoe_link_ok(lp)) | |
189 | - new_status |= FC_LINK_UP; | |
190 | + new_link_up = !fcoe_link_ok(lp); | |
191 | break; | |
192 | case NETDEV_REGISTER: | |
193 | break; | |
194 | default: | |
195 | FC_DBG("unknown event %ld call", event); | |
196 | } | |
197 | - if (lp->link_status != new_status) { | |
198 | - if ((new_status & FC_LINK_UP) == FC_LINK_UP) | |
199 | + if (lp->link_up != new_link_up) { | |
200 | + if (new_link_up) | |
201 | fc_linkup(lp); | |
202 | else { | |
203 | stats = lp->dev_stats[smp_processor_id()]; | |
204 | --- a/drivers/scsi/libfc/fc_exch.c | |
205 | +++ b/drivers/scsi/libfc/fc_exch.c | |
206 | @@ -1096,7 +1096,7 @@ static void fc_exch_recv_abts(struct fc_ | |
207 | ap->ba_high_seq_cnt = fh->fh_seq_cnt; | |
208 | ap->ba_low_seq_cnt = htons(sp->cnt); | |
209 | } | |
210 | - sp = fc_seq_start_next(sp); | |
211 | + sp = fc_seq_start_next_locked(sp); | |
212 | spin_unlock_bh(&ep->ex_lock); | |
213 | fc_seq_send_last(sp, fp, FC_RCTL_BA_ACC, FC_TYPE_BLS); | |
214 | fc_frame_free(rx_fp); | |
215 | --- a/drivers/scsi/libfc/fc_fcp.c | |
216 | +++ b/drivers/scsi/libfc/fc_fcp.c | |
217 | @@ -20,13 +20,13 @@ | |
218 | */ | |
219 | ||
220 | #include <linux/module.h> | |
221 | +#include <linux/delay.h> | |
222 | #include <linux/kernel.h> | |
223 | #include <linux/types.h> | |
224 | #include <linux/spinlock.h> | |
225 | #include <linux/scatterlist.h> | |
226 | #include <linux/err.h> | |
227 | #include <linux/crc32.h> | |
228 | -#include <linux/delay.h> | |
229 | ||
230 | #include <scsi/scsi_tcq.h> | |
231 | #include <scsi/scsi.h> | |
232 | @@ -1622,7 +1622,7 @@ out: | |
233 | static inline int fc_fcp_lport_queue_ready(struct fc_lport *lp) | |
234 | { | |
235 | /* lock ? */ | |
236 | - return (lp->state == LPORT_ST_READY) && (lp->link_status & FC_LINK_UP); | |
237 | + return (lp->state == LPORT_ST_READY) && lp->link_up && !lp->qfull; | |
238 | } | |
239 | ||
240 | /** | |
241 | @@ -1891,7 +1891,7 @@ int fc_eh_abort(struct scsi_cmnd *sc_cmd | |
242 | lp = shost_priv(sc_cmd->device->host); | |
243 | if (lp->state != LPORT_ST_READY) | |
244 | return rc; | |
245 | - else if (!(lp->link_status & FC_LINK_UP)) | |
246 | + else if (!lp->link_up) | |
247 | return rc; | |
248 | ||
249 | spin_lock_irqsave(lp->host->host_lock, flags); | |
250 | --- a/drivers/scsi/libfc/fc_lport.c | |
251 | +++ b/drivers/scsi/libfc/fc_lport.c | |
252 | @@ -250,7 +250,7 @@ void fc_get_host_port_state(struct Scsi_ | |
253 | { | |
254 | struct fc_lport *lp = shost_priv(shost); | |
255 | ||
256 | - if ((lp->link_status & FC_LINK_UP) == FC_LINK_UP) | |
257 | + if (lp->link_up) | |
258 | fc_host_port_state(shost) = FC_PORTSTATE_ONLINE; | |
259 | else | |
260 | fc_host_port_state(shost) = FC_PORTSTATE_OFFLINE; | |
261 | @@ -484,7 +484,7 @@ static void fc_lport_recv_rnid_req(struc | |
262 | * @sp: current sequence in the ADISC exchange | |
263 | * @fp: ADISC request frame | |
264 | * | |
265 | - * Locking Note: The lport lock is exected to be held before calling | |
266 | + * Locking Note: The lport lock is expected to be held before calling | |
267 | * this function. | |
268 | */ | |
269 | static void fc_lport_recv_adisc_req(struct fc_seq *sp, struct fc_frame *in_fp, | |
270 | @@ -577,8 +577,8 @@ void fc_linkup(struct fc_lport *lport) | |
271 | fc_host_port_id(lport->host)); | |
272 | ||
273 | mutex_lock(&lport->lp_mutex); | |
274 | - if ((lport->link_status & FC_LINK_UP) != FC_LINK_UP) { | |
275 | - lport->link_status |= FC_LINK_UP; | |
276 | + if (!lport->link_up) { | |
277 | + lport->link_up = 1; | |
278 | ||
279 | if (lport->state == LPORT_ST_RESET) | |
280 | fc_lport_enter_flogi(lport); | |
281 | @@ -597,8 +597,8 @@ void fc_linkdown(struct fc_lport *lport) | |
282 | FC_DEBUG_LPORT("Link is down for port (%6x)\n", | |
283 | fc_host_port_id(lport->host)); | |
284 | ||
285 | - if ((lport->link_status & FC_LINK_UP) == FC_LINK_UP) { | |
286 | - lport->link_status &= ~(FC_LINK_UP); | |
287 | + if (lport->link_up) { | |
288 | + lport->link_up = 0; | |
289 | fc_lport_enter_reset(lport); | |
290 | lport->tt.fcp_cleanup(lport); | |
291 | } | |
292 | @@ -607,30 +607,6 @@ void fc_linkdown(struct fc_lport *lport) | |
293 | EXPORT_SYMBOL(fc_linkdown); | |
294 | ||
295 | /** | |
296 | - * fc_pause - Pause the flow of frames | |
297 | - * @lport: The lport to be paused | |
298 | - */ | |
299 | -void fc_pause(struct fc_lport *lport) | |
300 | -{ | |
301 | - mutex_lock(&lport->lp_mutex); | |
302 | - lport->link_status |= FC_PAUSE; | |
303 | - mutex_unlock(&lport->lp_mutex); | |
304 | -} | |
305 | -EXPORT_SYMBOL(fc_pause); | |
306 | - | |
307 | -/** | |
308 | - * fc_unpause - Unpause the flow of frames | |
309 | - * @lport: The lport to be unpaused | |
310 | - */ | |
311 | -void fc_unpause(struct fc_lport *lport) | |
312 | -{ | |
313 | - mutex_lock(&lport->lp_mutex); | |
314 | - lport->link_status &= ~(FC_PAUSE); | |
315 | - mutex_unlock(&lport->lp_mutex); | |
316 | -} | |
317 | -EXPORT_SYMBOL(fc_unpause); | |
318 | - | |
319 | -/** | |
320 | * fc_fabric_logoff - Logout of the fabric | |
321 | * @lport: fc_lport pointer to logoff the fabric | |
322 | * | |
323 | @@ -977,7 +953,7 @@ static void fc_lport_enter_reset(struct | |
324 | fc_host_fabric_name(lport->host) = 0; | |
325 | fc_host_port_id(lport->host) = 0; | |
326 | ||
327 | - if ((lport->link_status & FC_LINK_UP) == FC_LINK_UP) | |
328 | + if (lport->link_up) | |
329 | fc_lport_enter_flogi(lport); | |
330 | } | |
331 | ||
332 | --- a/drivers/scsi/libfc/fc_rport.c | |
333 | +++ b/drivers/scsi/libfc/fc_rport.c | |
334 | @@ -425,7 +425,7 @@ static void fc_rport_error(struct fc_rpo | |
335 | PTR_ERR(fp), fc_rport_state(rport), rdata->retries); | |
336 | ||
337 | if (!fp || PTR_ERR(fp) == -FC_EX_TIMEOUT) { | |
338 | - /* | |
339 | + /* | |
340 | * Memory allocation failure, or the exchange timed out. | |
341 | * Retry after delay | |
342 | */ | |
343 | --- a/include/scsi/libfc.h | |
344 | +++ b/include/scsi/libfc.h | |
345 | @@ -68,9 +68,6 @@ | |
346 | /* | |
347 | * FC HBA status | |
348 | */ | |
349 | -#define FC_PAUSE (1 << 1) | |
350 | -#define FC_LINK_UP (1 << 0) | |
351 | - | |
352 | enum fc_lport_state { | |
353 | LPORT_ST_NONE = 0, | |
354 | LPORT_ST_FLOGI, | |
355 | @@ -603,7 +600,8 @@ struct fc_lport { | |
356 | ||
357 | /* Operational Information */ | |
358 | struct libfc_function_template tt; | |
359 | - u16 link_status; | |
360 | + u8 link_up; | |
361 | + u8 qfull; | |
362 | enum fc_lport_state state; | |
363 | unsigned long boot_time; | |
364 | ||
365 | @@ -704,12 +702,6 @@ void fc_linkup(struct fc_lport *); | |
366 | void fc_linkdown(struct fc_lport *); | |
367 | ||
368 | /* | |
369 | - * Pause and unpause traffic. | |
370 | - */ | |
371 | -void fc_pause(struct fc_lport *); | |
372 | -void fc_unpause(struct fc_lport *); | |
373 | - | |
374 | -/* | |
375 | * Configure the local port. | |
376 | */ | |
377 | int fc_lport_config(struct fc_lport *); |