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
> > >
> >
>
