Re: [PATCH] cxl/hdm: Avoid NULL deref when component registers are missing

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 30 Mar 2023 11:49:26 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Thu, 30 Mar 2023 11:19:42 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> > > Jonathan Cameron wrote:  
> > > > On Wed, 29 Mar 2023 14:35:43 -0700
> > > > Dan Williams <dan.j.williams@intel.com> wrote:
> > > >     
> > > > > The cxl_port driver attempts to support endpoint devices that do not
> > > > > advertise a component register block, but by inspection
> > > > > devm_cxl_setup_hdm() passes a NULL @crb to helper functions that should
> > > > > be skipped.
> > > > > 
> > > > > Return early and skip setting target_count since that is only relevant
> > > > > for switch decoders, not endpoint decoders.    
> > > > 
> > > > This is a good observation. It would be nice to not read it for the
> > > > HDM decoder path either. Obviously we don't use it so that doesn't do
> > > > any harm, but to someone reading the code it looks like we care about the
> > > > value.  I'm not immediately sure how we'd establish at this layer that
> > > > the HDM decoder is a switch or HB one though..    
> > > 
> > > @info is NULL when this routine is called for non-endpoint decoders.  
> > 
> > Ah, so we could pass a flag into parse_hdm_decoder_caps() and not read
> > the value if it has no meaning for the particular decoder.  
> 
> How about kerneldoc on 'struct cxl_hdm' clarifying @target_count and
> other fields, because I don't see the benefit of logic to skip parsing
> that field. The overhead of an MMIO cycle to read the capability
> register has already been spent.
> 

Ok. I guess it's harmless even it if gains meaning in some later spec
version as we don't use it for anything.

Jonathan