>-----Original Message----- >From: Intel-wired-lan [mailto:intel-wired-lan-boun...@osuosl.org] On >Behalf Of Tantilov, Emil S >Sent: Thursday, February 01, 2018 4:58 PM >To: Shannon Nelson <shannon.nel...@oracle.com> >Cc: netdev@vger.kernel.org; intel-wired-...@lists.osuosl.org >Subject: Re: [Intel-wired-lan] Possible read-modify-write bug in ixgbe >x550 phy setup > >>-----Original Message----- >>From: Shannon Nelson [mailto:shannon.nel...@oracle.com] >>Sent: Thursday, February 01, 2018 4:44 PM >>To: Tantilov, Emil S <emil.s.tanti...@intel.com> >>Cc: netdev@vger.kernel.org; intel-wired-...@lists.osuosl.org >>Subject: Re: [Intel-wired-lan] Possible read-modify-write bug in ixgbe >>x550 phy setup >> >>On 2/1/2018 4:34 PM, Tantilov, Emil S wrote: >>>> -----Original Message----- >>>> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@osuosl.org] On >>>> Behalf Of Shannon Nelson >>>> Sent: Thursday, February 01, 2018 3:46 PM >>>> To: Tantilov, Emil S <emil.s.tanti...@intel.com> >>>> Cc: netdev@vger.kernel.org; intel-wired-...@lists.osuosl.org >>>> Subject: [Intel-wired-lan] Possible read-modify-write bug in ixgbe >>x550 >>>> phy setup >>>> >>>> Hi Emil, >>>> >>>> I was looking through a set of ixgbe patches and came across this >>commit >>>> >>>> commit 410a494902777c11f95031d9ed757d7f8f09c5c6 >>>> ixgbe: add write flush when configuring CS4223/7 >>>> >>>> and am wondering about the setting of reg_phy_ext in the middle of >>>> ixgbe_setup_mac_link_sfp_x550a(). It looks like it is read from the >>>> PHY, modified to remove the CX1 and SR mode bits, but then those >bits >>>> are overwritten in the "if (setup_linear)" block immediately >>following, >>>> and that is what gets written back out. >>> >>> Hi Shannon, >>> >>> This is pretty standard clear before set, so you're right that it >>would >>> make more sense to have |= rather than =. >>> >>> Are you seeing an issue, or did you catch this via code inspection? >> >>Purely a code inspection, I saw this while looking at something else. >> >>sln > >Thanks for letting us know. I will do some more testing with the actual >HW before changing the code.
Just FYI - we looked at the reads and confirmed that there is no functional bug in the code because as it happens the CX1/SR bits is the only bits that are read and set and as such we don't lose any data. This of course means that the read is not needed and we'll just remove it. Thanks, Emil
