fs/sysfs: MIPS: Don't translate all IOCTLs #402

svenschwermer · 2019-02-13T20:46:35Z

Some IOCTLs are fixed, e.g. i2c-dev. There, we must not translate the
IOCTLs. Instead, the IOCTLs are generated arch-dependent in the fs
package.

Fixes #361

googlebot · 2019-02-13T20:46:38Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2019-02-13T20:46:38Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

svenschwermer · 2019-02-13T20:48:47Z

I signed it!

googlebot · 2019-02-13T20:48:49Z

CLAs look good, thanks!

googlebot · 2019-02-13T20:48:49Z

CLAs look good, thanks!

maruel

Thanks a lot for verifying and working on a fix. That's really appreciated.

Exporting these function was something I discussed with @NeuralSpaz but wasn't sure about it up to now.

Please try to make the travis check pass.

host/fs/ioctl.go

host/sysfs/spi.go

codecov-io · 2019-02-14T18:37:07Z

Codecov Report

Merging #402 into master will increase coverage by 0.31%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##           master     #402      +/-   ##
==========================================
+ Coverage   64.39%   64.71%   +0.31%     
==========================================
  Files         118      116       -2     
  Lines       11806    11726      -80     
==========================================
- Hits         7603     7588      -15     
+ Misses       4001     3937      -64     
+ Partials      202      201       -1

Impacted Files	Coverage Δ
host/videocore/videocore.go	`81.08% <ø> (ø)`	⬆️
host/sysfs/spi.go	`83.98% <40%> (+0.71%)`	⬆️
host/fs/fs_linux.go
host/sysfs/thermal_sensor.go	`83.84% <0%> (+2.3%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a024e5...f7eb31d. Read the comment docs.

host/fs/ioctl.go

host/sysfs/spi.go

maruel · 2019-02-14T20:22:41Z

host/sysfs/spi_test.go

+	}
+}
+
+func TestSPIIOCTx(t *testing.T) {


That test should fail on mips, right? We should delete this then, because anyway we have gohci workers that ensure that SPI works for real.

You're right, they do fail. I verified this by cross-compiling the test and then running it on the target. I wasn't sure how to test this. Should we include arch-specific unit tests or just rely on the gohci tests?

No need to test for the constants, integration testing will suffice here. Maybe the only thing that would be cool is to add a test to spismoketest: read the values back via an ioctl on the file handle to confirm that the values were setup properly; that's the reason why the "read value back to confirm" code was written, as a long time ago the code was trying to write value and was silently failing (oops).

FTR, I don't ask you to do the spismoketest check here, what you did is already great.

I still think the dead code and const checks should be removed.

I added a check to read back the mode flag since that's the only flag we set during initialization. SPI_IOC_MESSAGE doesn't have a corresponding _IOR ioctl, so we can't verify the message ioctl.
I'm unsure if the GetFlag approach is the right one here, what do you think?

maruel · 2019-02-14T20:24:04Z

gohci

host/fs/ioctl.go

maruel · 2019-02-16T01:19:59Z

host/sysfs/spi_test.go

+	}
+}
+
+func TestSPIIOCTx(t *testing.T) {


FTR, I don't ask you to do the spismoketest check here, what you did is already great.

I still think the dead code and const checks should be removed.

Some IOCTLs are fixed, e.g. i2c-dev. There, we must not translate the IOCTLs. Instead, the IOCTLs are generated arch-dependent in the fs package.

Beside being clearer, the comment in the code was incorrect. The message box magic is in fact 'd' = 100 = 0x64, not 0x100.

host/videocore/videocore.go

maruel

It's great, will merge a bit later I have to go

maruel · 2019-02-17T18:48:46Z

Sorry for the delay, but can you fix https://github.com/google/periph/blob/master/host/sysfs/i2c.go#L205 too? Otherwise I²C will be broken.

maruel · 2019-02-17T18:48:53Z

gohci

svenschwermer · 2019-02-18T07:06:27Z

Sorry for the delay, but can you fix https://github.com/google/periph/blob/master/host/sysfs/i2c.go#L205 too? Otherwise I²C will be broken.

Are you sure? The whole point of this PR was to not translate the IOCTLs for I2C on MIPS because they're fixed and not dependent on the _IO{R,W,} macros. See https://github.com/torvalds/linux/blob/master/include/uapi/linux/i2c-dev.h#L36

Perhaps I am misunderstanding you here?

maruel · 2019-02-18T12:52:35Z

Duh, sorry.

maruel · 2019-02-18T12:54:37Z

\o/

svenschwermer mentioned this pull request Feb 13, 2019

sysfs: Omega2+ I²C failure due to MIPS ioctl encoding #361

Closed

maruel reviewed Feb 14, 2019

View reviewed changes

host/fs/ioctl.go Outdated Show resolved Hide resolved

host/fs/ioctl.go Outdated Show resolved Hide resolved

host/sysfs/spi.go Show resolved Hide resolved

host/sysfs/spi.go Outdated Show resolved Hide resolved

svenschwermer force-pushed the master branch from d6a0b04 to 7079ed8 Compare February 14, 2019 18:15

maruel reviewed Feb 14, 2019

View reviewed changes

maruel reviewed Feb 16, 2019

View reviewed changes

svenschwermer added 2 commits February 16, 2019 16:25

MIPS: Don't translate all IOCTLs

a76dd93

Some IOCTLs are fixed, e.g. i2c-dev. There, we must not translate the IOCTLs. Instead, the IOCTLs are generated arch-dependent in the fs package.

videocore: Replace ioctl magic with derived value

d6aa45c

Beside being clearer, the comment in the code was incorrect. The message box magic is in fact 'd' = 100 = 0x64, not 0x100.

svenschwermer force-pushed the master branch from 4c3cccd to d6aa45c Compare February 16, 2019 15:26

maruel reviewed Feb 16, 2019

View reviewed changes

host/videocore/videocore.go Outdated Show resolved Hide resolved

maruel approved these changes Feb 16, 2019

View reviewed changes

videocore: Use dynamic pointer size for ioctl

f7eb31d

maruel merged commit df7c7a4 into google:master Feb 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs/sysfs: MIPS: Don't translate all IOCTLs #402

fs/sysfs: MIPS: Don't translate all IOCTLs #402

svenschwermer commented Feb 13, 2019

googlebot commented Feb 13, 2019

googlebot commented Feb 13, 2019

svenschwermer commented Feb 13, 2019

googlebot commented Feb 13, 2019

googlebot commented Feb 13, 2019

maruel left a comment

codecov-io commented Feb 14, 2019 •

edited

Loading

maruel Feb 14, 2019

svenschwermer Feb 14, 2019

maruel Feb 14, 2019

maruel Feb 16, 2019

svenschwermer Feb 16, 2019

maruel commented Feb 14, 2019

maruel Feb 16, 2019

maruel left a comment

maruel commented Feb 17, 2019

maruel commented Feb 17, 2019

svenschwermer commented Feb 18, 2019

maruel commented Feb 18, 2019

maruel commented Feb 18, 2019

fs/sysfs: MIPS: Don't translate all IOCTLs #402

fs/sysfs: MIPS: Don't translate all IOCTLs #402

Conversation

svenschwermer commented Feb 13, 2019

googlebot commented Feb 13, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Feb 13, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

svenschwermer commented Feb 13, 2019

googlebot commented Feb 13, 2019

googlebot commented Feb 13, 2019

maruel left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 14, 2019 • edited Loading

Codecov Report

maruel Feb 14, 2019

Choose a reason for hiding this comment

svenschwermer Feb 14, 2019

Choose a reason for hiding this comment

maruel Feb 14, 2019

Choose a reason for hiding this comment

maruel Feb 16, 2019

Choose a reason for hiding this comment

svenschwermer Feb 16, 2019

Choose a reason for hiding this comment

maruel commented Feb 14, 2019

maruel Feb 16, 2019

Choose a reason for hiding this comment

maruel left a comment

Choose a reason for hiding this comment

maruel commented Feb 17, 2019

maruel commented Feb 17, 2019

svenschwermer commented Feb 18, 2019

maruel commented Feb 18, 2019

maruel commented Feb 18, 2019

codecov-io commented Feb 14, 2019 •

edited

Loading