]>
Commit | Line | Data |
---|---|---|
4d1e5b62 AF |
1 | From: Robert Love <robert.w.love@intel.com> |
2 | Subject: libfc: Ensure correct device_put/get usage (round 2) | |
3 | References: | |
4 | ||
5 | Reference counting was barely used and where used | |
6 | it was incorrect. This patch creates a few simple | |
7 | policies. | |
8 | ||
9 | When the rport->dev [e.g. struct device] is initialized | |
10 | it starts with a refcnt of 1. Whenever we're using the | |
11 | rport we will increment the count. When we logoff we | |
12 | should decrement the count to 0 and the 'release' | |
13 | function will be called. The FC transport provides the | |
14 | release function for real rports and libfc provides it | |
15 | for rogue rports. When we switch from a rogue to real | |
16 | rport we'll decrement the refcnt on the rogue rport | |
17 | and increment it for the real rport, after we've created | |
18 | it. | |
19 | ||
20 | Any externally initiated action on an rport (login, | |
21 | logoff) will not require the caller to increment and | |
22 | decrement the refcnt. | |
23 | ||
24 | For rport_login(), the rport will have just been created | |
25 | and therefore no other thread would be able to access | |
26 | this object. | |
27 | ||
28 | For rport_logoff(), the rport will have been removed | |
29 | from the list of rports and therefore no other thread | |
30 | would be able to lookup() this rport. | |
31 | ||
32 | This patch removes the get_device() from the rport_lookup | |
33 | function. These are the places where it is called and why | |
34 | we don't need a reference. | |
35 | ||
36 | fc_disc_recv_rscn_req() - called for single port RSCNs | |
37 | the disc mutex is held and | |
38 | ensures that no other thread | |
39 | will find this rport. | |
40 | ||
41 | fc_disc_new_target() - Same. The rport cannot be looked up | |
42 | so no other thread can free the rport. | |
43 | This code looks buggy though, we | |
44 | shouldn't be calling rport_login() on | |
45 | a 'real' rport, which we could do. | |
46 | ||
47 | fc_disc_single() - Same. disc mutex protects the list. | |
48 | ||
49 | fc_lport_recv_req() - Similar, but this time the lport lock | |
50 | ensures that no incoming requests are | |
51 | processed until the current request | |
52 | for an rport has returned. | |
53 | ||
54 | When the rport layer needs to send a request it will | |
55 | increment the count so that the EM can be confident that | |
56 | the rport is present when making the callback. If | |
57 | fc_remote_port_delete() is called before the response | |
58 | callback, which is often the case for LOGO commands, the | |
59 | refcnt will still have a value of 1 becuase we grabbed the | |
60 | lock before the ctels_send() is called. The exchange would | |
61 | have been removed and so the callback will be called with | |
62 | an error code. After processing the error code we'll | |
63 | decrement the refcnt for the last time and the rport will | |
64 | be free'd. | |
65 | ||
66 | Since point-to-point mode is not working this patch | |
67 | does not consider point-to-point. | |
68 | ||
69 | Signed-off-by: Robert Love <robert.w.love@intel.com> | |
70 | Acked-by: Bernhard Walle <bwalle@suse.de> | |
71 | --- | |
72 | ||
73 | drivers/scsi/libfc/fc_disc.c | 5 +---- | |
74 | drivers/scsi/libfc/fc_lport.c | 5 ++--- | |
75 | drivers/scsi/libfc/fc_rport.c | 21 ++++++++++++++------- | |
76 | 3 files changed, 17 insertions(+), 14 deletions(-) | |
77 | ||
78 | ||
79 | --- a/drivers/scsi/libfc/fc_disc.c | |
80 | +++ b/drivers/scsi/libfc/fc_disc.c | |
81 | @@ -81,7 +81,6 @@ struct fc_rport *fc_disc_lookup_rport(co | |
82 | if (rport->port_id == port_id) { | |
83 | disc_found = 1; | |
84 | found = rport; | |
85 | - get_device(&found->dev); | |
86 | break; | |
87 | } | |
88 | } | |
89 | @@ -767,10 +766,8 @@ static void fc_disc_single(struct fc_dis | |
90 | goto out; | |
91 | ||
92 | rport = lport->tt.rport_lookup(lport, dp->ids.port_id); | |
93 | - if (rport) { | |
94 | + if (rport) | |
95 | fc_disc_del_target(disc, rport); | |
96 | - put_device(&rport->dev); /* hold from lookup */ | |
97 | - } | |
98 | ||
99 | new_rport = fc_rport_rogue_create(dp); | |
100 | if (new_rport) { | |
101 | --- a/drivers/scsi/libfc/fc_lport.c | |
102 | +++ b/drivers/scsi/libfc/fc_lport.c | |
103 | @@ -908,10 +908,9 @@ static void fc_lport_recv_req(struct fc_ | |
104 | d_id = ntoh24(fh->fh_d_id); | |
105 | ||
106 | rport = lport->tt.rport_lookup(lport, s_id); | |
107 | - if (rport) { | |
108 | + if (rport) | |
109 | lport->tt.rport_recv_req(sp, fp, rport); | |
110 | - put_device(&rport->dev); /* hold from lookup */ | |
111 | - } else { | |
112 | + else { | |
113 | rjt_data.fp = NULL; | |
114 | rjt_data.reason = ELS_RJT_UNAB; | |
115 | rjt_data.explan = ELS_EXPL_NONE; | |
116 | --- a/drivers/scsi/libfc/fc_rport.c | |
117 | +++ b/drivers/scsi/libfc/fc_rport.c | |
118 | @@ -111,16 +111,11 @@ struct fc_rport *fc_rport_rogue_create(s | |
119 | rport->roles = dp->ids.roles; | |
120 | rport->maxframe_size = FC_MIN_MAX_PAYLOAD; | |
121 | /* | |
122 | - * init the device, so other code can manipulate the rport as if | |
123 | - * it came from the fc class. We also do an extra get because | |
124 | - * libfc will free this rport instead of relying on the normal | |
125 | - * refcounting. | |
126 | - * | |
127 | * Note: all this libfc rogue rport code will be removed for | |
128 | * upstream so it fine that this is really ugly and hacky right now. | |
129 | */ | |
130 | device_initialize(&rport->dev); | |
131 | - get_device(&rport->dev); | |
132 | + rport->dev.release = fc_rport_rogue_destroy; // XXX: bwalle | |
133 | ||
134 | mutex_init(&rdata->rp_mutex); | |
135 | rdata->local_port = dp->lp; | |
136 | @@ -402,9 +397,9 @@ static void fc_rport_timeout(struct work | |
137 | case RPORT_ST_NONE: | |
138 | break; | |
139 | } | |
140 | - put_device(&rport->dev); | |
141 | ||
142 | mutex_unlock(&rdata->rp_mutex); | |
143 | + put_device(&rport->dev); | |
144 | } | |
145 | ||
146 | /** | |
147 | @@ -531,6 +526,7 @@ out: | |
148 | fc_frame_free(fp); | |
149 | err: | |
150 | mutex_unlock(&rdata->rp_mutex); | |
151 | + put_device(&rport->dev); | |
152 | } | |
153 | ||
154 | /** | |
155 | @@ -562,6 +558,8 @@ static void fc_rport_enter_plogi(struct | |
156 | if (!lport->tt.elsct_send(lport, rport, fp, ELS_PLOGI, | |
157 | fc_rport_plogi_resp, rport, lport->e_d_tov)) | |
158 | fc_rport_error(rport, fp); | |
159 | + else | |
160 | + get_device(&rport->dev); | |
161 | } | |
162 | ||
163 | /** | |
164 | @@ -631,6 +629,7 @@ out: | |
165 | fc_frame_free(fp); | |
166 | err: | |
167 | mutex_unlock(&rdata->rp_mutex); | |
168 | + put_device(&rport->dev); | |
169 | } | |
170 | ||
171 | /** | |
172 | @@ -679,6 +678,7 @@ out: | |
173 | fc_frame_free(fp); | |
174 | err: | |
175 | mutex_unlock(&rdata->rp_mutex); | |
176 | + put_device(&rport->dev); | |
177 | } | |
178 | ||
179 | /** | |
180 | @@ -712,6 +712,8 @@ static void fc_rport_enter_prli(struct f | |
181 | if (!lport->tt.elsct_send(lport, rport, fp, ELS_PRLI, | |
182 | fc_rport_prli_resp, rport, lport->e_d_tov)) | |
183 | fc_rport_error(rport, fp); | |
184 | + else | |
185 | + get_device(&rport->dev); | |
186 | } | |
187 | ||
188 | /** | |
189 | @@ -777,6 +779,7 @@ out: | |
190 | fc_frame_free(fp); | |
191 | err: | |
192 | mutex_unlock(&rdata->rp_mutex); | |
193 | + put_device(&rport->dev); | |
194 | } | |
195 | ||
196 | /** | |
197 | @@ -806,6 +809,8 @@ static void fc_rport_enter_rtv(struct fc | |
198 | if (!lport->tt.elsct_send(lport, rport, fp, ELS_RTV, | |
199 | fc_rport_rtv_resp, rport, lport->e_d_tov)) | |
200 | fc_rport_error(rport, fp); | |
201 | + else | |
202 | + get_device(&rport->dev); | |
203 | } | |
204 | ||
205 | /** | |
206 | @@ -835,6 +840,8 @@ static void fc_rport_enter_logo(struct f | |
207 | if (!lport->tt.elsct_send(lport, rport, fp, ELS_LOGO, | |
208 | fc_rport_logo_resp, rport, lport->e_d_tov)) | |
209 | fc_rport_error(rport, fp); | |
210 | + else | |
211 | + get_device(&rport->dev); | |
212 | } | |
213 | ||
214 |