On 27/03/2017 23:07, Marc Zyngier wrote: > On Mon, Mar 27 2017 at 08:44:08 PM, Mason wrote: > >> On 27/03/2017 19:09, Marc Zyngier wrote: >> >>> On 27/03/17 16:53, Mason wrote: >>> >>>> I have one remaining issue with bitmaps. >>>> >>>> My HW regs are 32b. How do I grab e.g. bits 96-127? >>>> All I can think of is >>>> u32 val = ((u32 *)bitmap)[3]; >>>> >>>> Is this acceptable? >>> >>> No. >>> >>>> mrutland mentioned bitmap_to_u32array() but IIUC it is used to >>>> copy an entire bitmap. >>> >>> The real question is "Why do you need such a thing?". >> >> You told me to use an in-memory version of the "unmasked" >> bitmap, yes? In that case, I need to be able to grab >> a piece of said bitmap, to update the corresponding piece >> of the HW bitmap. >> >> For example, assume the first 3 MSIs are unmasked, >> in other words, unmasked = 0x7 >> >> A new user comes along and wants to assign an MSI. >> Scan "unmasked", found bit at pos 3. >> Update "unmasked" to 0xf. >> At some point, I need to write 0xf to some HW reg. >> So I need to grab a piece of "unmasked" (bits 0-31 in my example) >> >> pos = find_first_zero_bit(unmasked, COUNT); >> __set_bit(pos, unmasked); >> int reg_index = pos / 32; >> u32 val = ((u32 *)unmasked)[reg_index]; >> writel_relaxed(val, pcie->enabled + reg_index * 4); >> >> Or did I miss something in your suggestion? > > I don't know, I'm sightly taken aback by your question. Completely > puzzled, actually. "Read Modify Write" is a fairly obvious construct. > > val = readl_relaxed(pcie->enabled + reg_index); > writel_relaxed(val | BIT(pos % 32), pcie->enabled + reg_index); > > I never realized this could be such a novel concept. Replace pos with > hwirq, add a spinlock, and that's your irq_unmask. I'm not sure why you would think I'm not familiar with RMW. My original code was: mask = readl_relaxed(pcie->msi_mask); pos = find_first_zero_bit(&mask, 32); writel(mask | BIT(pos), pcie->msi_mask); To which you replied: "Do you really need to read this from the HW each time you allocate an interrupt? That feels pretty crazy. You're much better off having an in-memory bitmap that will make things more efficient" This is why I was trying to avoid a MMIO read from the HW each time I allocate an interrupt. > My suggestion was to use a bitmap in order not to perform extra MMIO > accesses on the fast path. It doesn't mean that you cannot read from the > register under any circumstances. It just means that you don't do it > when there are more efficient ways to do it. The fast path is the interrupt handler, right? (I didn't read the interrupt mask in the ISR.) I understand your underlying point. It is fine to read from MMIO in the slow path, but try as hard as possible NOT to do so in the fast path. Regards.
