On Sun, Jul 7, 2013 at 10:15 AM, Amitkumar Karwar <akarwar@xxxxxxxxxxx> wrote: > >>We didn't test the behavior of sdio_read/sdio_write API's when hardware is >not present. But they should just return an error. > > I ran some tests which confirmed that there is no harm in calling these API's when hardware is removed. They returned ENOMEDIUM error. > > Can you please review and verify attached patch? I gave it a quick test, didn't see any crashes. Thanks. However, I'm concerned that this patch is worsening the confusion in this part of the driver. Unless there is a reason to do otherwise, the norm here would be to tightly couple the hardware bits that enable interrupts to the Linux IRQ handler registration. i.e. sdio_claim_irq should be called immediately before mwifiex_sdio_enable_host_int, and sdio_release_irq should be called immediately after mwifiex_sdio_disable_host_int. Right now these 2 steps are very separate (sdio_release_irq is being called far too late, long after the point when the driver could sanely handle an interrupt). Although you would never expect an interrupt to arrive after mwifiex_sdio_disable_host_int() has been called, the cleanliness and readability has value here, and maybe damaged hardware would misbehave and fire an interrupt anyway, which would cause the driver to go wrong. Secondly, it should be symmetrical. If the core mwifiex driver is the thing that enables interrupts, it should be the component responsible for disabling them as well. With your patch, the core mwifiex driver enables interrupts, but it is up to the sdio sub-driver to disable them. Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
