RE: [PATCH v5 03/11] wifi: rtlwifi: Add rtl8192du/hw.{c,h}

[Ping-Ke Shih <pkshih@realtek.com>]

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> 
> On 10/05/2024 06:04, Ping-Ke Shih wrote:
> > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> >
> >> +
> >> +int rtl92du_hw_init(struct ieee80211_hw *hw)
> >> +{
> >> +       struct rtl_usb_priv *usb_priv = rtl_usbpriv(hw);
> >> +       struct rtl_usb *rtlusb = rtl_usbdev(usb_priv);
> >> +       struct rtl_priv *rtlpriv = rtl_priv(hw);
> >> +       struct rtl_ps_ctl *ppsc = rtl_psc(rtlpriv);
> >> +       struct rtl_hal *rtlhal = rtl_hal(rtlpriv);
> >> +       struct rtl_mac *mac = rtl_mac(rtlpriv);
> >> +       struct rtl_phy *rtlphy = &rtlpriv->phy;
> >> +       u8 val8, txpktbuf_bndy;
> >> +       unsigned long flags;
> >> +       int err, i;
> >> +       u32 val32;
> >> +       u16 val16;
> >> +
> >> +       /* As this function can take a very long time
> >> +        * and can be called with irqs disabled, reenable the irqs
> >> +        * to let the other devices continue being serviced.
> >> +        *
> >> +        * It is safe doing so since our own interrupts will only be enabled
> >> +        * in a subsequent step.
> >> +        */
> >> +       local_save_flags(flags);
> >> +       local_irq_enable();
> >
> > I think you just copy and paste this chunk. I wonder if USB has the same behavior?
> > Driver force to enable IRQ is weird to me. Please check I this is really called
> > with irq disabled? If not, we can remove them.
> >
> 
> Yes, I copied that. Removing it from rtl8192du makes no
> difference for me. I'm not sure how to check if the function
> is called with irq disabled. Is it raw_irqs_disabled() ?
> It returns 0 for me.
> 

I can't find an exact description about irqs_disabled() under Documentation/.
Then I did google search and found a page [1] that says
	Returns nonzero if local interrupt delivery is disabled; otherwise returns zero

I feel it is disallowed to call might sleeping API (like mutext()) when IRQ is
disabled. However, original code does. Take rtl92se_hw_init() as an example

rtl_op_start()
	mutex_lock(&rtlpriv->locks.conf_mutex);
	.adapter_start = rtl_pci_start()
		rtlpriv->cfg->ops->hw_init(hw)
			.hw_init = rtl92se_hw_init()
				local_irq_enable();
	mutex_unlock(&rtlpriv->locks.conf_mutex);


callers hold lps_mutex or ips_mutex
	rtl_ps_set_rf_state()
		.set_rf_power_state = rtl92s_phy_set_rf_power_state()
			rtl_ps_enable_nic()
				rtlpriv->cfg->ops->hw_init(hw)
						.hw_init = rtl92se_hw_init()
							local_irq_enable();

That means this drivers might no need local_irq_enable() nowadays. Currently
we can only remove it from USB adapters as first step. 


> > [...]
> >
> >> +       local_irq_disable();
> >> +       local_irq_restore(flags);
> >> +
> >> +       return err;
> >> +}
> >> +
> >
> >

[1] https://litux.nl/mirror/kerneldevelopment/0672327201/ch06lev1sec7.html