[Intel-wired-lan] [PATCH 04/10] igc: Refactor XDP rxq info registration

[Maciej Fijalkowski]

On Tue, Dec 22, 2020 at 09:43:35AM -0800, Andre Guedes wrote:
> Quoting Maciej Fijalkowski (2020-12-22 04:32:05)
> > > > > @@ -536,6 +539,9 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
> > > > >       u32 srrctl = 0, rxdctl = 0;
> > > > >       u64 rdba = ring->dma;
> > > > >  
> > > > > +     WARN_ON(xdp_rxq_info_reg_mem_model(&ring->xdp_rxq,
> > > > > +                                        MEM_TYPE_PAGE_SHARED, NULL));
> > > > 
> > > > You should do the unroll in case it fails just like it was done in
> > > > igc_xdp_register_rxq_info.
> > > 
> > > This was inspired in ixgbe driver.
> > > 
> > > The only reason xdp_rxq_info_reg_mem_model() could fail here is if xdp_rxq
> > > wasn't registered. However, this is very unlikely to happen since it is
> > > registered in igc_setup_rx_resources() which is always called before
> > > igc_configure_rx_ring(). The WARN_ON() macro is used just in case.
> > 
> > Agreed on that but let's not disregard the other failure cases that can
> > happen by saying that it can only fail when xdp_rxq wasn't registered.
> > 
> > I believe that functions returning statuses have been written in such way
> > for some reason, so I feel that ignoring error statuses is a wrong
> > attitude.
> > 
> > For example, igc_setup_all_rx_resources return value is ignored in
> > igc_request_irq, but in __igc_open it is checked. Why?
> > 
> > One last thing is that all other drivers besides igb/ixgbe do the error
> > handling.
> 
> Agreed. I have another series with some fixes to the driver that I'm working
> on. I can fix the issue with igc_request_irq() on that series.
> 
> > > If we really want to unroll, we should propagate the error back in the call
> > > chain, changing the returning type of igc_configure_rx_ring() as well as the
> > > other functions in the call chain, so the unrolling is done in the proper
> > > place.
> > > 
> > > IMO, such change isn't worth it. It seems like a lot of change to cover a case
> > > that is never expected. WARN_ON() sound more suitable in those cases. Also,
> > > ixgbe is around for quite some time and this doesn't seem to be an issue.
> > 
> > Well, although I don't like it, I agree :P
> > 
> > The uncomfortable question would be what will happen if we fail to
> > register that mem model but due to the fact that driver is written in a
> > way that it is not profitable to do unrolling?
> 
> I see your point. In that case, I think we can tackle that in a separate series
> changing all three drivers (igc, ixgbe, and igb). Sounds good?

Sure :)