Re: [PATCH v2 1/2] switchtec: Fix false maximum supported PCIe function number issue

[Bjorn Helgaas]

On Fri, Apr 19, 2019 at 12:56:21AM +0800, wesleywesley wrote:
> On Wed, Apr 17, 2019 at 05:48:58PM -0500, Bjorn Helgaas wrote:
> > On Mon, Apr 15, 2019 at 10:41:41PM +0800, Wesley Sheng wrote:
> > > The hardware supports up to 255 PFFs and the driver only supports 48, so
> > > this patch updates the driver to support them all. To be backward
> > > compatible, a new ioctl and corresponding data structure are created,
> > > while keep the deprecated one.
> > 
> > The commit log needs to include some kind of detail about what "PFF"
> > is.  Logan provided the following, which looks like a good starting
> > point:
> > 
> >   PFF is really a concept internal to the Switchtec device. It stands
> >   for PCIe Function Framework. Essentially, there is a bank of
> >   registers for every PCIe Function (aka endpoint) in the switch. When
> >   I originally wrote the driver, I assumed incorrectly there would
> >   only ever be one PFF per port and the maximum number of ports for
> >   Switchtec parts is 48. In fact, the hardware supports up to 255 and
> >   there are typically two PFFs per upstream port (one for the port
> >   itself and one for the management endpoint).
> > 
> > I had a couple minor questions below that aren't exactly related to
> > this patch.  If you did decide to address them (in a separate patch,
> > of course), you could expand the commit log for this patch with the
> > PFF info.  If the questions below don't need anything, I can
> > incorporate something about PFF myself.
> 
> Hi, Bjorn,
> 
> See my comments below your questions.
> They are good catches, but I think it is not a bug we should fix it urgently.
> What about take it as an improvement of code, and we will submit it in the
> next upstream session. Before that, we would like to test with this change.

Totally agreed, I'll merge the current stuff today and we can get on with
this.

> > > ---
> > >  drivers/pci/switch/switchtec.c       | 39 +++++++++++++++++++++++++-----------
> > >  include/linux/switchtec.h            |  2 +-
> > >  include/uapi/linux/switchtec_ioctl.h | 13 +++++++++++-
> > >  3 files changed, 40 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
> > > index e22766c..7df9a69 100644
> > > --- a/drivers/pci/switch/switchtec.c
> > > +++ b/drivers/pci/switch/switchtec.c
> > > @@ -658,19 +658,25 @@ static int ioctl_flash_part_info(struct switchtec_dev *stdev,
> > >  
> > >  static int ioctl_event_summary(struct switchtec_dev *stdev,
> > >  	struct switchtec_user *stuser,
> > > -	struct switchtec_ioctl_event_summary __user *usum)
> > > +	struct switchtec_ioctl_event_summary __user *usum,
> > > +	size_t size)
> > >  {
> > > -	struct switchtec_ioctl_event_summary s = {0};
> > > +	struct switchtec_ioctl_event_summary *s;
> > >  	int i;
> > >  	u32 reg;
> > > +	int ret = 0;
> > >  
> > > -	s.global = ioread32(&stdev->mmio_sw_event->global_summary);
> > > -	s.part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
> > > -	s.local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
> > > +	s = kzalloc(sizeof(*s), GFP_KERNEL);
> > > +	if (!s)
> > > +		return -ENOMEM;
> > > +
> > > +	s->global = ioread32(&stdev->mmio_sw_event->global_summary);
> > > +	s->part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
> > > +	s->local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
> > >  
> > >  	for (i = 0; i < stdev->partition_count; i++) {
> > >  		reg = ioread32(&stdev->mmio_part_cfg_all[i].part_event_summary);
> > > -		s.part[i] = reg;
> > > +		s->part[i] = reg;
> > >  	}
> > >  
> > >  	for (i = 0; i < SWITCHTEC_MAX_PFF_CSR; i++) {
> > 
> > Should this be "i < stdev->pff_csr_count", as in
> > check_link_state_events(), enable_link_state_events() and
> > mask_all_events()?  If so, I assume the read and check of vendor_id
> > would be unnecessary?
> 
> Yes, I think it is a good catch.
> Once the configuration file of Switchtec is choosed, the logical ports bound to 
> USPs/DSPs physical port, unbound logical ports, and vEPs (NT/Management/NT only),
> their corresponding PFF's index are fixed and the total number are invariable.
> > 
> > The last loop in init_pff() currently checks against
> > SWITCHTEC_MAX_PFF_CSR:
> > 
> >         for (i = 0; i < ARRAY_SIZE(pcfg->dsp_pff_inst_id); i++) {
> >                 reg = ioread32(&pcfg->dsp_pff_inst_id[i]);
> >                 if (reg < SWITCHTEC_MAX_PFF_CSR)
> >                         stdev->pff_local[reg] = 1;
> >         }
> > 
> > Should it check "reg < stdev->pff_csr_count" instead?  It looks like
> > mask_all_events(), the only reader of pff_local[], only looks up to
> > pff_csr_count anyway.
> > 
> Yes, I agree.
> Actually, all the SWITCHTEC_MAX_PFF_CSR in init_pff() could be replaced by it.
> > > @@ -679,15 +685,19 @@ static int ioctl_event_summary(struct switchtec_dev *stdev,
> > >  			break;
> > >  
> > >  		reg = ioread32(&stdev->mmio_pff_csr[i].pff_event_summary);
> > > -		s.pff[i] = reg;
> > > +		s->pff[i] = reg;
> > >  	}
> > >  
> > > -	if (copy_to_user(usum, &s, sizeof(s)))
> > > -		return -EFAULT;
> > > +	if (copy_to_user(usum, s, size)) {
> > > +		ret = -EFAULT;
> > > +		goto error_case;
> > > +	}
> > >  
> > >  	stuser->event_cnt = atomic_read(&stdev->event_cnt);
> > >  
> > > -	return 0;
> > > +error_case:
> > > +	kfree(s);
> > > +	return ret;
> > >  }
> > >  
> > >  static u32 __iomem *global_ev_reg(struct switchtec_dev *stdev,
> > > @@ -977,8 +987,9 @@ static long switchtec_dev_ioctl(struct file *filp, unsigned int cmd,
> > >  	case SWITCHTEC_IOCTL_FLASH_PART_INFO:
> > >  		rc = ioctl_flash_part_info(stdev, argp);
> > >  		break;
> > > -	case SWITCHTEC_IOCTL_EVENT_SUMMARY:
> > > -		rc = ioctl_event_summary(stdev, stuser, argp);
> > > +	case SWITCHTEC_IOCTL_EVENT_SUMMARY_LEGACY:
> > > +		rc = ioctl_event_summary(stdev, stuser, argp,
> > > +					 sizeof(struct switchtec_ioctl_event_summary_legacy));
> > >  		break;
> > >  	case SWITCHTEC_IOCTL_EVENT_CTL:
> > >  		rc = ioctl_event_ctl(stdev, argp);
> > > @@ -989,6 +1000,10 @@ static long switchtec_dev_ioctl(struct file *filp, unsigned int cmd,
> > >  	case SWITCHTEC_IOCTL_PORT_TO_PFF:
> > >  		rc = ioctl_port_to_pff(stdev, argp);
> > >  		break;
> > > +	case SWITCHTEC_IOCTL_EVENT_SUMMARY:
> > > +		rc = ioctl_event_summary(stdev, stuser, argp,
> > > +					 sizeof(struct switchtec_ioctl_event_summary));
> > > +		break;
> > >  	default:
> > >  		rc = -ENOTTY;
> > >  		break;
> > > diff --git a/include/linux/switchtec.h b/include/linux/switchtec.h
> > > index 52a079b..0cfc34a 100644
> > > --- a/include/linux/switchtec.h
> > > +++ b/include/linux/switchtec.h
> > > @@ -20,7 +20,7 @@
> > >  #include <linux/cdev.h>
> > >  
> > >  #define SWITCHTEC_MRPC_PAYLOAD_SIZE 1024
> > > -#define SWITCHTEC_MAX_PFF_CSR 48
> > > +#define SWITCHTEC_MAX_PFF_CSR 255
> > >  
> > >  #define SWITCHTEC_EVENT_OCCURRED BIT(0)
> > >  #define SWITCHTEC_EVENT_CLEAR    BIT(0)
> > > diff --git a/include/uapi/linux/switchtec_ioctl.h b/include/uapi/linux/switchtec_ioctl.h
> > > index 4f4daf8..c912b5a 100644
> > > --- a/include/uapi/linux/switchtec_ioctl.h
> > > +++ b/include/uapi/linux/switchtec_ioctl.h
> > > @@ -50,7 +50,7 @@ struct switchtec_ioctl_flash_part_info {
> > >  	__u32 active;
> > >  };
> > >  
> > > -struct switchtec_ioctl_event_summary {
> > > +struct switchtec_ioctl_event_summary_legacy {
> > >  	__u64 global;
> > >  	__u64 part_bitmap;
> > >  	__u32 local_part;
> > > @@ -59,6 +59,15 @@ struct switchtec_ioctl_event_summary {
> > >  	__u32 pff[48];
> > >  };
> > >  
> > > +struct switchtec_ioctl_event_summary {
> > > +	__u64 global;
> > > +	__u64 part_bitmap;
> > > +	__u32 local_part;
> > > +	__u32 padding;
> > > +	__u32 part[48];
> > > +	__u32 pff[255];
> > > +};
> > > +
> > >  #define SWITCHTEC_IOCTL_EVENT_STACK_ERROR		0
> > >  #define SWITCHTEC_IOCTL_EVENT_PPU_ERROR			1
> > >  #define SWITCHTEC_IOCTL_EVENT_ISP_ERROR			2
> > > @@ -127,6 +136,8 @@ struct switchtec_ioctl_pff_port {
> > >  	_IOWR('W', 0x41, struct switchtec_ioctl_flash_part_info)
> > >  #define SWITCHTEC_IOCTL_EVENT_SUMMARY \
> > >  	_IOR('W', 0x42, struct switchtec_ioctl_event_summary)
> > > +#define SWITCHTEC_IOCTL_EVENT_SUMMARY_LEGACY \
> > > +	_IOR('W', 0x42, struct switchtec_ioctl_event_summary_legacy)
> > >  #define SWITCHTEC_IOCTL_EVENT_CTL \
> > >  	_IOWR('W', 0x43, struct switchtec_ioctl_event_ctl)
> > >  #define SWITCHTEC_IOCTL_PFF_TO_PORT \
> > > -- 
> > > 2.7.4
> > > 
> > 
>