Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Danilo,

> On 14 May 2025, at 18:53, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Wed, May 14, 2025 at 04:20:51PM -0300, Daniel Almeida wrote:
>> +/// // This is running in process context.
>> +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
>> +///     let registration = Registration::register(irq, flags::SHARED, c_str!("my-device"), handler);
>> +///
>> +///     // You can have as many references to the registration as you want, so
>> +///     // multiple parts of the driver can access it.
>> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
> 
> This makes it possible to arbitrarily extend the lifetime of an IRQ
> registration. However, we must guarantee that the IRQ is unregistered when the
> corresponding device is unbound. We can't allow drivers to hold on to device
> resources after the corresponding device has been unbound.
> 
> Why does the data need to be part of the IRQ registration itself? Why can't we
> pass in an Arc<T> instance already when we register the IRQ?
> 
> This way we'd never have a reason to ever access the Registration instance
> itself ever again and we can easily wrap it as Devres<irq::Registration> -
> analogously to devm_request_irq() on the C side - without any penalties.
> 
>> +///     // The handler may be called immediately after the function above
>> +///     // returns, possibly in a different CPU.
>> +///
>> +///     {
>> +///         // The data can be accessed from the process context too.
>> +///         let mut data = registration.handler().0.lock();
>> +///         *data = 42;
>> +///     }
>> +///
>> +///     Ok(registration)
>> +/// }
> 

Up until this point, there was no need for the data to not be inline with the
registration. This new design would force an Arc, which, apart from the
heap-allocation, is restrictive for users.

Can’t we use Devres with the current implementation?

IIUC from a very cursory glance, all that would mean is that you'd have to call
try_access() on your handler, which should be fine?

— Daniel