On Fri, 4 Oct 2019 09:39:34 +0300
Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> wrote:
> Hi,
>
> Jonathan Cameron <jonathan.cameron@xxxxxxxxxx> writes:
> >> >> +static int intel_adc_read_raw(struct iio_dev *iio,
> >> >> + struct iio_chan_spec const *channel, int *val, int *val2,
> >> >> + long mask)
> >> >> +{
> >> >> + struct intel_adc *adc = iio_priv(iio);
> >> >> + int shift;
> >> >> + int ret;
> >> >> +
> >> >> + switch (mask) {
> >> >> + case IIO_CHAN_INFO_RAW:
> >> >> + shift = channel->scan_type.shift;
> >> >> +
> >> >> + ret = iio_device_claim_direct_mode(iio);
> >> >> + if (ret)
> >> >> + break;
> >> >> +
> >> >> + intel_adc_enable(adc);
> >> >> +
> >> >> + ret = intel_adc_single_channel_conversion(adc, channel, val);
> >> >> + if (ret) {
> >> >> + intel_adc_disable(adc);
> >> >> + iio_device_release_direct_mode(iio);
> >> >> + break;
> >> >
> >> > nitpick (feel free to ignore).
> >> > It might be nice to pull this case block as a separate function, then you
> >> > could cleanly use goto to do the unwinding.
> >>
> >> you mean something like below:
> >>
> >> static int intel_adc_read_info_raw(...)
> >> {
> >> ....
> >> }
> >>
> >> static int intel_adc_read_raw(...)
> >> {
> >> switch (mask) {
> >> case IIO_CHAN_INFO_RAW:
> >> ret = intel_adc_read_info_raw(...);
> >> break;
> >> default:
> >> ret = -EINVAL;
> >> }
> >> }
> >>
> >> ??
> >
> > Yes, exactly that.
>
> I'll change it, no worries.
>
> >> >> + ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
> >> >> + if (ret < 0)
> >> >> + return ret;
> >> >> +
> >> >> + irq = pci_irq_vector(pci, 0);
> >> >> + ret = devm_request_irq(&pci->dev, irq, intel_adc_irq,
> >> >> + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING,
> >> >> + "intel-adc", adc);
> >> >
> >> > Requesting the interrupt only after exposing userspace and in kernel
> >> > interfaces seems liable to cause problem.
> >>
> >> It goes the other way around, rather. If I request the interrupt before,
> >> then I could get interrupts before IIO subsystem knows about the device,
> >> no?
> >
> > Only if your device comes up with interrupts already enabled. Normally they
> > only turn on in response to some userspace interaction, such as enabling
> > a threshold. Unless there is a hardware limitation, then at startup no
> > such interrupt sources should be enabled.
>
> We have FW that _may_ use the hardware and leave it at unpredictable
> state. There is a potential for irq status bits being left over by
> FW.
>
If it is only status bits rather than actually leaving the interrupt enabled
I'd do whatever actions are needed to clear those so you are in a
clean state when the driver loads (basically do the equivalent of what
you would get if there was a "soft reset" function.
Unpredictable is nasty! :)
Jonathan
>
> >> >> + if (ret)
> >> >> + goto err;
> >> >> +
> >> >> + pm_runtime_set_autosuspend_delay(&pci->dev, 1000);
> >> >> + pm_runtime_use_autosuspend(&pci->dev);
> >> >> + pm_runtime_put_autosuspend(&pci->dev);
> >> >> + pm_runtime_allow(&pci->dev);
> >> >> +
> >> >> + return 0;
> >> >> +
> >> >> +err:
> >> >> + pci_free_irq_vectors(pci);
> >> >> + return ret;
> >> >> +}
> >> >> +
> >> >> +static void intel_adc_remove(struct pci_dev *pci)
> >> >> +{
> >> >> + pm_runtime_forbid(&pci->dev);
> >> >> + pm_runtime_get_noresume(&pci->dev);
> >> >> +
> >> >> + pci_free_irq_vectors(pci);
> >> >
> >> > There is a theoretical race here. We have freed the irq vectors
> >> > before removing the userspace and in kernel interfaces.
> >>
> >> There's no way to sort this out, though. Is there? Apart from switching
> >> away from device managed resources.
> >
> > There is the rather helpful,
> >
> > devm_add_action_or_reset() that allows you to define additional cleanup
> > actions to be automatically run. It's either that, or stop using
> > device managed resources from the point at which something that isn't
> > device managed occurs in probe.
>
> I'll have a look, thanks.
>
