Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
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(>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
Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
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(>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
Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
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(>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
Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
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(>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(>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(>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, _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, _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, _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, _start,
size, err);
306 goto err_out;
307 }
308 break;
309 }
310
311 if (pa != pa_start)
312
[bug report] IB/usnic: Add Cisco VIC low-level hardware driver
[ 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(>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(>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(>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, _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, _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, _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, _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(>list,
321 struct
usnic_uiom_chunk,
322
