* [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
@ 2021-07-05 10:23 Dan Carpenter
2021-07-05 13:47 ` Robin Murphy
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2021-07-05 10:23 UTC (permalink / raw)
To: umalhi; +Cc: linux-rdma, iommu
[ Ancient code, but the bug seems real enough still. -dan ]
Hello Upinder Malhi,
The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following static checker
warning:
drivers/iommu/iommu.c:2482 iommu_map()
warn: sleeping in atomic context
drivers/infiniband/hw/usnic/usnic_uiom.c
244 static int usnic_uiom_map_sorted_intervals(struct list_head *intervals,
245 struct usnic_uiom_reg *uiomr)
This function is always called from usnic_uiom_reg_get() which is holding
spin_lock(&pd->lock); so it can't sleep.
246 {
247 int i, err;
248 size_t size;
249 struct usnic_uiom_chunk *chunk;
250 struct usnic_uiom_interval_node *interval_node;
251 dma_addr_t pa;
252 dma_addr_t pa_start = 0;
253 dma_addr_t pa_end = 0;
254 long int va_start = -EINVAL;
255 struct usnic_uiom_pd *pd = uiomr->pd;
256 long int va = uiomr->va & PAGE_MASK;
257 int flags = IOMMU_READ | IOMMU_CACHE;
258
259 flags |= (uiomr->writable) ? IOMMU_WRITE : 0;
260 chunk = list_first_entry(&uiomr->chunk_list, struct usnic_uiom_chunk,
261 list);
262 list_for_each_entry(interval_node, intervals, link) {
263 iter_chunk:
264 for (i = 0; i < chunk->nents; i++, va += PAGE_SIZE) {
265 pa = sg_phys(&chunk->page_list[i]);
266 if ((va >> PAGE_SHIFT) < interval_node->start)
267 continue;
268
269 if ((va >> PAGE_SHIFT) == interval_node->start) {
270 /* First page of the interval */
271 va_start = va;
272 pa_start = pa;
273 pa_end = pa;
274 }
275
276 WARN_ON(va_start == -EINVAL);
277
278 if ((pa_end + PAGE_SIZE != pa) &&
279 (pa != pa_start)) {
280 /* PAs are not contiguous */
281 size = pa_end - pa_start + PAGE_SIZE;
282 usnic_dbg("va 0x%lx pa %pa size 0x%zx flags 0x%x",
283 va_start, &pa_start, size, flags);
284 err = iommu_map(pd->domain, va_start, pa_start,
285 size, flags);
The iommu_map() function sleeps.
286 if (err) {
287 usnic_err("Failed to map va 0x%lx pa %pa size 0x%zx with err %d\n",
288 va_start, &pa_start, size, err);
289 goto err_out;
290 }
291 va_start = va;
292 pa_start = pa;
293 pa_end = pa;
294 }
295
296 if ((va >> PAGE_SHIFT) == interval_node->last) {
297 /* Last page of the interval */
298 size = pa - pa_start + PAGE_SIZE;
299 usnic_dbg("va 0x%lx pa %pa size 0x%zx flags 0x%x\n",
300 va_start, &pa_start, size, flags);
301 err = iommu_map(pd->domain, va_start, pa_start,
302 size, flags);
iommu_map() again.
303 if (err) {
304 usnic_err("Failed to map va 0x%lx pa %pa size 0x%zx with err %d\n",
305 va_start, &pa_start, size, err);
306 goto err_out;
307 }
308 break;
309 }
310
311 if (pa != pa_start)
312 pa_end += PAGE_SIZE;
313 }
314
315 if (i == chunk->nents) {
316 /*
317 * Hit last entry of the chunk,
318 * hence advance to next chunk
319 */
320 chunk = list_first_entry(&chunk->list,
321 struct usnic_uiom_chunk,
322 list);
323 goto iter_chunk;
324 }
325 }
326
327 return 0;
328
329 err_out:
330 usnic_uiom_unmap_sorted_intervals(intervals, pd);
331 return err;
332 }
regards,
dan carpenter
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
2021-07-05 10:23 [bug report] IB/usnic: Add Cisco VIC low-level hardware driver Dan Carpenter
@ 2021-07-05 13:47 ` Robin Murphy
2021-07-05 15:21 ` Jason Gunthorpe
0 siblings, 1 reply; 5+ messages in thread
From: Robin Murphy @ 2021-07-05 13:47 UTC (permalink / raw)
To: Dan Carpenter, umalhi; +Cc: linux-rdma, iommu
On 2021-07-05 11:23, Dan Carpenter wrote:
> [ Ancient code, but the bug seems real enough still. -dan ]
>
> Hello Upinder Malhi,
>
> The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
> driver" from Sep 10, 2013, leads to the following static checker
> warning:
>
> drivers/iommu/iommu.c:2482 iommu_map()
> warn: sleeping in atomic context
>
> drivers/infiniband/hw/usnic/usnic_uiom.c
> 244 static int usnic_uiom_map_sorted_intervals(struct list_head *intervals,
> 245 struct usnic_uiom_reg *uiomr)
>
> This function is always called from usnic_uiom_reg_get() which is holding
> spin_lock(&pd->lock); so it can't sleep.
FWIW back in those days it wasn't really well defined whether
iommu_map() was callable from non-sleeping contexts (the arch/arm DMA
API code relied on it, for instance). It was only formalised 2 years ago
by 781ca2de89ba ("iommu: Add gfp parameter to iommu_ops::map") which
introduced the might_sleep() check that's firing there. I guess these
calls want to be updated to iommu_map_atomic() now.
Robin.
> 246 {
> 247 int i, err;
> 248 size_t size;
> 249 struct usnic_uiom_chunk *chunk;
> 250 struct usnic_uiom_interval_node *interval_node;
> 251 dma_addr_t pa;
> 252 dma_addr_t pa_start = 0;
> 253 dma_addr_t pa_end = 0;
> 254 long int va_start = -EINVAL;
> 255 struct usnic_uiom_pd *pd = uiomr->pd;
> 256 long int va = uiomr->va & PAGE_MASK;
> 257 int flags = IOMMU_READ | IOMMU_CACHE;
> 258
> 259 flags |= (uiomr->writable) ? IOMMU_WRITE : 0;
> 260 chunk = list_first_entry(&uiomr->chunk_list, struct usnic_uiom_chunk,
> 261 list);
> 262 list_for_each_entry(interval_node, intervals, link) {
> 263 iter_chunk:
> 264 for (i = 0; i < chunk->nents; i++, va += PAGE_SIZE) {
> 265 pa = sg_phys(&chunk->page_list[i]);
> 266 if ((va >> PAGE_SHIFT) < interval_node->start)
> 267 continue;
> 268
> 269 if ((va >> PAGE_SHIFT) == interval_node->start) {
> 270 /* First page of the interval */
> 271 va_start = va;
> 272 pa_start = pa;
> 273 pa_end = pa;
> 274 }
> 275
> 276 WARN_ON(va_start == -EINVAL);
> 277
> 278 if ((pa_end + PAGE_SIZE != pa) &&
> 279 (pa != pa_start)) {
> 280 /* PAs are not contiguous */
> 281 size = pa_end - pa_start + PAGE_SIZE;
> 282 usnic_dbg("va 0x%lx pa %pa size 0x%zx flags 0x%x",
> 283 va_start, &pa_start, size, flags);
> 284 err = iommu_map(pd->domain, va_start, pa_start,
> 285 size, flags);
>
> The iommu_map() function sleeps.
>
> 286 if (err) {
> 287 usnic_err("Failed to map va 0x%lx pa %pa size 0x%zx with err %d\n",
> 288 va_start, &pa_start, size, err);
> 289 goto err_out;
> 290 }
> 291 va_start = va;
> 292 pa_start = pa;
> 293 pa_end = pa;
> 294 }
> 295
> 296 if ((va >> PAGE_SHIFT) == interval_node->last) {
> 297 /* Last page of the interval */
> 298 size = pa - pa_start + PAGE_SIZE;
> 299 usnic_dbg("va 0x%lx pa %pa size 0x%zx flags 0x%x\n",
> 300 va_start, &pa_start, size, flags);
> 301 err = iommu_map(pd->domain, va_start, pa_start,
> 302 size, flags);
>
> iommu_map() again.
>
> 303 if (err) {
> 304 usnic_err("Failed to map va 0x%lx pa %pa size 0x%zx with err %d\n",
> 305 va_start, &pa_start, size, err);
> 306 goto err_out;
> 307 }
> 308 break;
> 309 }
> 310
> 311 if (pa != pa_start)
> 312 pa_end += PAGE_SIZE;
> 313 }
> 314
> 315 if (i == chunk->nents) {
> 316 /*
> 317 * Hit last entry of the chunk,
> 318 * hence advance to next chunk
> 319 */
> 320 chunk = list_first_entry(&chunk->list,
> 321 struct usnic_uiom_chunk,
> 322 list);
> 323 goto iter_chunk;
> 324 }
> 325 }
> 326
> 327 return 0;
> 328
> 329 err_out:
> 330 usnic_uiom_unmap_sorted_intervals(intervals, pd);
> 331 return err;
> 332 }
>
> regards,
> dan carpenter
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
2021-07-05 13:47 ` Robin Murphy
@ 2021-07-05 15:21 ` Jason Gunthorpe
2021-07-05 15:39 ` Robin Murphy
2021-07-05 15:45 ` Dan Carpenter
0 siblings, 2 replies; 5+ messages in thread
From: Jason Gunthorpe @ 2021-07-05 15:21 UTC (permalink / raw)
To: Robin Murphy, Christian Benvenuti, Nelson Escobar
Cc: linux-rdma, iommu, umalhi, Dan Carpenter
On Mon, Jul 05, 2021 at 02:47:36PM +0100, Robin Murphy wrote:
> On 2021-07-05 11:23, Dan Carpenter wrote:
> > [ Ancient code, but the bug seems real enough still. -dan ]
> >
> > Hello Upinder Malhi,
> >
> > The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
> > driver" from Sep 10, 2013, leads to the following static checker
> > warning:
> >
> > drivers/iommu/iommu.c:2482 iommu_map()
> > warn: sleeping in atomic context
> >
> > drivers/infiniband/hw/usnic/usnic_uiom.c
> > 244 static int usnic_uiom_map_sorted_intervals(struct list_head *intervals,
> > 245 struct usnic_uiom_reg *uiomr)
> >
> > This function is always called from usnic_uiom_reg_get() which is holding
> > spin_lock(&pd->lock); so it can't sleep.
>
> FWIW back in those days it wasn't really well defined whether iommu_map()
> was callable from non-sleeping contexts (the arch/arm DMA API code relied on
> it, for instance). It was only formalised 2 years ago by 781ca2de89ba
> ("iommu: Add gfp parameter to iommu_ops::map") which introduced the
> might_sleep() check that's firing there. I guess these calls want to be
> updated to iommu_map_atomic() now.
Does this mean this driver doesn't work at all upstream? I would be
quite interested to delete it.
Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
2021-07-05 15:21 ` Jason Gunthorpe
@ 2021-07-05 15:39 ` Robin Murphy
2021-07-05 15:45 ` Dan Carpenter
1 sibling, 0 replies; 5+ messages in thread
From: Robin Murphy @ 2021-07-05 15:39 UTC (permalink / raw)
To: Jason Gunthorpe, Christian Benvenuti, Nelson Escobar
Cc: linux-rdma, iommu, umalhi, Dan Carpenter
On 2021-07-05 16:21, Jason Gunthorpe wrote:
> On Mon, Jul 05, 2021 at 02:47:36PM +0100, Robin Murphy wrote:
>> On 2021-07-05 11:23, Dan Carpenter wrote:
>>> [ Ancient code, but the bug seems real enough still. -dan ]
>>>
>>> Hello Upinder Malhi,
>>>
>>> The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
>>> driver" from Sep 10, 2013, leads to the following static checker
>>> warning:
>>>
>>> drivers/iommu/iommu.c:2482 iommu_map()
>>> warn: sleeping in atomic context
>>>
>>> drivers/infiniband/hw/usnic/usnic_uiom.c
>>> 244 static int usnic_uiom_map_sorted_intervals(struct list_head *intervals,
>>> 245 struct usnic_uiom_reg *uiomr)
>>>
>>> This function is always called from usnic_uiom_reg_get() which is holding
>>> spin_lock(&pd->lock); so it can't sleep.
>>
>> FWIW back in those days it wasn't really well defined whether iommu_map()
>> was callable from non-sleeping contexts (the arch/arm DMA API code relied on
>> it, for instance). It was only formalised 2 years ago by 781ca2de89ba
>> ("iommu: Add gfp parameter to iommu_ops::map") which introduced the
>> might_sleep() check that's firing there. I guess these calls want to be
>> updated to iommu_map_atomic() now.
>
> Does this mean this driver doesn't work at all upstream? I would be
> quite interested to delete it.
I think the only time it's actually in trouble is on AMD hardware if one
of those iommu_map() calls has to allocate a new pagetable page and that
allocation has to go down whichever reclaim path actually sleeps.
Historically all the other IOMMU drivers it might have come into contact
with already used GFP_ATOMIC for their internal allocations anyway (AMD
was the only one using a mutex instead of a spinlock internally), and
some like intel-iommu still haven't relaxed that even now.
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
2021-07-05 15:21 ` Jason Gunthorpe
2021-07-05 15:39 ` Robin Murphy
@ 2021-07-05 15:45 ` Dan Carpenter
1 sibling, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-07-05 15:45 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Nelson Escobar, umalhi, linux-rdma, iommu, Robin Murphy,
Christian Benvenuti
On Mon, Jul 05, 2021 at 12:21:38PM -0300, Jason Gunthorpe wrote:
> On Mon, Jul 05, 2021 at 02:47:36PM +0100, Robin Murphy wrote:
> > On 2021-07-05 11:23, Dan Carpenter wrote:
> > > [ Ancient code, but the bug seems real enough still. -dan ]
> > >
> > > Hello Upinder Malhi,
> > >
> > > The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
> > > driver" from Sep 10, 2013, leads to the following static checker
> > > warning:
> > >
> > > drivers/iommu/iommu.c:2482 iommu_map()
> > > warn: sleeping in atomic context
> > >
> > > drivers/infiniband/hw/usnic/usnic_uiom.c
> > > 244 static int usnic_uiom_map_sorted_intervals(struct list_head *intervals,
> > > 245 struct usnic_uiom_reg *uiomr)
> > >
> > > This function is always called from usnic_uiom_reg_get() which is holding
> > > spin_lock(&pd->lock); so it can't sleep.
> >
> > FWIW back in those days it wasn't really well defined whether iommu_map()
> > was callable from non-sleeping contexts (the arch/arm DMA API code relied on
> > it, for instance). It was only formalised 2 years ago by 781ca2de89ba
> > ("iommu: Add gfp parameter to iommu_ops::map") which introduced the
> > might_sleep() check that's firing there. I guess these calls want to be
> > updated to iommu_map_atomic() now.
>
> Does this mean this driver doesn't work at all upstream? I would be
> quite interested to delete it.
It just means it hasn't been used with CONFIG_DEBUG_ATOMIC_SLEEP enabled
within the past two years.
regards,
dan carpenter
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-05 15:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-07-05 10:23 [bug report] IB/usnic: Add Cisco VIC low-level hardware driver Dan Carpenter
2021-07-05 13:47 ` Robin Murphy
2021-07-05 15:21 ` Jason Gunthorpe
2021-07-05 15:39 ` Robin Murphy
2021-07-05 15:45 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox