Microsemi / switchtec-kernel

A kernel module for the Microsemi PCIe switch
GNU General Public License v2.0
45 stars 31 forks source link

pff csr gas region support max 255 pcie functions instead of 48 #34

Closed wesleywesley closed 6 years ago

lsgunth commented 6 years ago

This is still not acceptable. It is not forwards and backwards compatible.

Existing switchtec-user binaries will send an ioctl with only 48 slots and the kernel will try to copy a structure that's larger than that resulting in bad things happening. An updated switchtec-user binary will similarly try to send a larger structure to an old kernel that only supports a shorter one without being aware.

wesleywesley commented 6 years ago

both #33 and #34 have backward & forward compatible issue with switchtec-user, when they have different size of array pff[] in struct switchtec_ioctl_event_summary.

but we do have to solve the case: when valid pff's number in csr is larger than 48, and in the first/header 48 pffs, it includes vep(mgt/nt ep +usp case) and unbind port, while other pffs, which is corresponding to bind port and nt only ep, located after the first 48 pffs.

I check the port event summary register definition in dev spec, it only use the bit 0 to bit 12. maybe we can use the MS Byte of pff (byte 3), to store the instance id of the pff, and keep the pff number in struct switchtec_ioctl_event_summary at 48, all of these 48 pff[] include only the valid port event summary register values among all the pffs. the max bind port number is 48 is the fact we can base. But this method is highly depend on firmware implementation.

lsgunth commented 6 years ago

No, you can't do that, the current switchtec_user code does not handle extra bits in the pff.

You need to essentially create two ioctls, a current and a deprecated one. The size of the struct is encoded in the ioctl number. If userspace submits the old ioctl, use the old struct; if they submit the new ioctl use the new struct.

Userspace will then need to try the new ioctl, and if it fails, use the old ioctl.

lsgunth commented 6 years ago

Also, if where making a change like that: think really hard if there's something else we might need to change about that ioctl at the same time.

wesleywesley commented 6 years ago

@lsgunth thx for reply.

If deprecated one is correct, we can keep it and create a current one. But the deprecated is created based on wrong input (instance id range is 0- 48, while the right case is 0-254), therefore, it will introduce error results on some circumstance.What I suggested is remove the deprecated, avoid/forbid the error case.

Compare to compatibility, I think error case is more unacceptable. While compatibility issue, we will inform user to update both driver and user tool together.

lsgunth commented 6 years ago

No, you seem to be missing the one rule you cannot break: by changing the kernel you cannot make existing programs no longer work. This includes old versions of updated software.

We've gone for a long time with only 48 pffs so if someone actually runs into this problem you can tell them to upgrade both pieces of software to fix it. But it's absolutely no excuse to break peoples working systems if they don't upgrade their kernel in software at the same time.

wesleywesley commented 6 years ago

43 : switchtec: Fix false maximum supported PCIe function number issue

solving the same problem with taking backward compatibility into account, close this one accordingly.