On Tue, 05 Oct 2021 14:15:45 +0100, Pali Rohár pali@kernel.org wrote:
Hello!
I dislike this approach. It adds another magic number which is just causing issues. Please read commit message for patch 11/13 where we describe why such magic constants are bad and already caused lot of issues in this driver.
As I said, feel free to write something better.
/* Process MSI interrupts */ if (isr0_status & PCIE_ISR0_MSI_INT_PENDING) advk_pcie_handle_msi(pcie); /* Process legacy interrupts */
- for (i = 0; i < PCI_NUM_INTX; i++) {
if (!(isr1_status & PCIE_ISR1_INTX_ASSERT(i)))continue;
- for_each_set_bit(i, &isr1_status, PCI_NUM_INTX) { advk_writel(pcie, PCIE_ISR1_INTX_ASSERT(i), PCIE_ISR1_REG);
- what you are doing here is code cleanup. We are currently in the state where we have lots of fixes for this driver, which we are hoping will go also to stable.
Yes, it is code cleanup. Because I don't find this patch to be very good, TBH. As for going into stable, that's not relevant for this discussion.
Some of them depend on these changes. Can we please first apply those fixes (we want to send them in batches to avoid sending 60 patchs in one series, since last time nobody wanted to review all of that) and do this afterwards?
It would be better to start with patches that are in a better shape. After all, this is what the code review process is about. This isn't "just take my patches".
- you are throwing away lower 8 bits of isr1_status. We have follow-up patches (not in this series, but in another batch which we want to send after this) that will be using those lower 8 bits, so we do not want to throw away them now.
I'm discarding these bits because *in isolation*, that's the correct thing to do. Feel free to propose a better patch that doesn't discard these bits and still makes the code more palatable.
The code pattern in this function is: compose irs*_status variable and then compare it with register macros defined at the top of driver. Each bit in this register represent some event and for each event there is simple macro to match.
So with your proposed change it would break all macros (as they are going to be shifted by magic constant) and then this code disallow access to events represented by low bits. And also it makes code pattern different for isr0_status and isr1_status variables which is very confusing and probably source for introduction of new bugs.
Read what I have said: I'm suggesting changes based on this patch *in isolation*. I don't see any other related patch in my inbox (nor do I want to receive any). Feel free to suggest something better (that's the third time I write this...).
Also the whole early-return optimization can be removed as it does not change functionality. So we will do so.
But we do not agree with the lower 8 bit discard of the isr1_status variable as explained above.
So if we add the explanation to commit message and drop the early return, would it be ok?
Do what you want. I have no interest in this particular piece of code, and only replied to Lorenzo's request. It doesn't change what I think of this patch, but I have too much on my plate to get dragged into sterile arguments.
M.