[DEVS-ONLY] SuperSU developer discussion

Chainfire · Sep 5, 2014

THIS THREAD IS FOR DEVELOPERS ONLY. IF YOU ARE NOT A DEVELOPER (or very tech-savvy and well-versed), MOST LIKELY YOU SHOULD NOT POST HERE.

By request, I am creating this thread for developer discussion. This is the place for developers to ask questions about how to handle/implement/embed SuperSU, discuss the operation of SuperSU, suggest improvements to compatibility, etc.

This thread hopefully reduces important developer related matters from being buried in the more user-oriented threads.

Please always include the version number of SuperSU you are referring to, even if it's the latest version right now. If applicable, also include information about phone and firmware you are testing with.

garyd9 · Jan 5, 2016

Chainfire said:
The stop-gap solution is to disable this caching completely, which is what the 000000deepsleep script does, which you can find mentioned or quoted in many posts around the forum. From SuperSU 2.66 onwards, that script is automatically installed on Samsung devices when systemless root is used.

Please forgive me for posting (in a cf-auto-root thread) and digging afterwards. I had thought I'd just dump the info and forget about it, but I couldn't stop digging...

...which led me to the quoted material.

Digging in the supersu 2.66 update-~~script~~binary, I see that you're detecting "samsung" in the build fingerprint, and if true, doing a systemless install AND applying a deepsleep fix. This works for Galaxy S6 devices, but not for some other similar platform devices. In particular, the Note5 has THREE devices that need caching disabled in order for deep sleep to function. (0:0:0:3 as well as :2 and :1.)

My first question is: does the SGS6 even have a file named "/sys/class/scsi_disk/0:0:0:3/cache_type"? If not, just write to all three files and don't worry about it. The third write would fail on the SGS6 and all would be good. It'd be no worse of a work-around than already exists (and I think it's a bad work-around.)

If that file DOES exist in the SGS6, then something would have caching turned off that really shouldn't. Of course, existing or not, automatically tossing in this deepsleep patch for every single device that has "samsung" in the build fingerprint would seem likely break proper caching in some yet unknown samsung device. Perhaps the SGS7 will change things up so that :1 should be left cache flushable, and :2 would be the only one that should block cache flushing.

As well, it's also possible that Samsung will pull in the kernel fix to resolve this issue before they release Android M. (Okay, perhaps it'd be more likely for Samsung to open source touchwiz... but we can always have fantasies.)

My problem with the work-around is expressed above: it can break something in the future (and cause a support headache when some ONE exynos7420 device is fixed, but the rest aren't.) As well, it sets precedent of having platform specific hacks in the generic update.zip (but only allowing for a single platform and not in an easily expandable way.)

Obviously, it would be a maintenance nightmare to have different "00000deepsleep" files for every different device model. (if 'zerolte.*', SGS6. If 'nobellte.*', Note5, etc.)...

In keeping with what I tell other people, I feel I now have an obligation to suggest A Better Way. (a person shouldn't complain about something unless they can make a reasonable suggestion on how it'd be better.)

So, here's my slightly convoluted (but expandable) suggestion:

You currently use /data/.supersu to read certain variables that modify the supersu.zip installer script. Perhaps those "platform specific lines" could be added to that file, and the installer script would put them in place. So, I could do the following in a recovery root shell before installing supersu.zip:

Code:

echo PLATFORMSTARTUP='echo "temporary none" > /sys/class/scsi_disk/0:0:0:1/cache_type' >> /data/.supersu

(I'd have included both (or all three) needed lines for samsung deep sleep, but I forget how to include CR in a shell cmdline.. )

Then, the supersu installer script would just read PLATFORMSTARTUP and write it's contents to /su/su.d/00000platformstartup (and set perms.)
Given this type of thing, the existing 000000deepsleep hack would be removed. Then, individual devs could easily create simplistic "pre-installers" for supersu for specific platforms that need changes. Those "pre" installers would just write the needed PLATFORMSTARTUP lines to /data/.supersu...

... and then supersu.zip no longer needs platform specific hacks.

Some random XDA developer could then generate a simple "SGS6-supersu.zip" would only contain an edify script to mount /data and add/edit the .supersu file's PLATFORMSTARTUP variable to contain the two lines needed for deep sleep (and another dev could write a Note5 for the 3 lines needed on that platform... and so on..)

Take care
Gary

NZgeek · Jan 5, 2016

garyd9 said:
You currently use /data/.supersu to read certain variables that modify the supersu.zip installer script. Perhaps those "platform specific lines" could be added to that file, and the installer script would put them in place. So, I could do the following in a recovery root shell before installing supersu.zip:

...

The only problem with that is that it requires users to have two brain cells to rub together. We've seen time and time again on these forums that you can't assume this is always the case.

I think that Chainfire is doing pretty much the right thing here. At worst, disabling write-back caching will make I/O a bit slower, but that's better than not having deep sleep. The only suggestion I'd have is to add more devices (maybe up to 5), and to check for their existence before writing to them.

garyd9 · Jan 5, 2016

NZgeek said:
The only problem with that is that it requires users to have two brain cells to rub together. We've seen time and time again on these forums that you can't assume this is always the case.

I think that Chainfire is doing pretty much the right thing here. At worst, disabling write-back caching will make I/O a bit slower, but that's better than not having deep sleep.

The problems with the existing solution are:
1. It blindly alters the system kernel behavior for every single device samsung manufactures.
2. It only actually does any good for a single one of the dozens of devices from that sam manufacturer.
3. It completely ignores every OTHER device that might need a bit of help (and potentially does more harm than good for those devices.)
4. It encourages device developers (users on XDA) to download SuperSU.zip and re-package it to have device specific hacks in the .zip archive (creating a mess.)

Actually, I don't think I need to explain all the problems with the existing hack. I'd imagine (hope) that it was done as something quick to test out an idea, and was never intended to be left in place in it's current form.

NZgeek said:
The only suggestion I'd have is to add more devices (maybe up to 5), and to check for their existence before writing to them.

Which 5 devices? Who maintains that list? Who updates it for each firmware change that might require an update? Will there be a new "SuperSU.zip" package each time a firmware change on a device requires that one of those 5 be changed? Who deals with the support nightmare of saying "use SuperSU v a.bc for device X firmwawre Y" and "superSU v d.ef for device X firmware Z", etc?

My proposed solution takes all the device-specific stuff completely out of the SuperSU package. It changes it from a device-specific solution to be a more generic and expandable solution that requires LESS support from SuperSU and places the device specific burden outside.

Instead of encouraging device developers to repackage supersu to device specific packages, it encourages device developers to package something else alongside supersu that would work with the existing (and unaltered) supersu.zip (and would be future compatible.)

Take care
Gary

nkk71 · Jan 8, 2016

spiral777 said:
would there be a way to get kexec/ multirom working with systemless root?

and would flashing a modified boot image to a rom also effect the kexec hardboot partch of the kernel?

1- the current versions of systemless root make changes/additions to the kernel, but you're not "flashing a modified boot img", so kexec is not broken, since the kernel is in essence the same as before

2- yes it is possible for systemless root (tested with 2.65) to get it working on multirom, however some changes were needed; we're still debugging the problem to try to narrow down the issue, to get it to work with as little changes as possible

EDIT: I'll just mention the problems encountered in case @Chainfire wants to be aware of them
a) line 1170: dd if=/dev/zero of=$BOOTIMAGE bs=4096
since MultiROM creates a symlink, the above command actually starts nulling out a "dummy boot.img" file, which basically continues on, untill all free space in internal storage (or external sdcard where applicable) is filled out

b) when MultiROM-TWRP finishes installing SuperSU, the fake /data is still "busy" (some open file or something else keeping it busy), since it's busy, it can't be unmounted properly, and the real mount points don't get restored
at that point mrom injection will fail
using a lazy unmount helped alleviate that (as a workaround), but obviously not the best solution

c) the setprop sukernel.mount 1 (in launch_daemonsu.sh) doesn't trigger the mount properly, workaround was to mount it in the launch_daemonsu.sh using "mount -t ext4 -o loop /data/su.img /su" instead of the setprop

EDIT2: thanks @Captain_Throwback for the reminder
d) the attempted remount read-only, will cause a bootloop; workaround: had to comment that out

just FYI, but I'll check more thoroughly when I get a chance

Chainfire · Jan 12, 2016

@garyd9 @NZgeek

Some good points are raised. I am not going to go into them all individually.

There is one core point of disagreement though. While I do not think device-specific patches generally have a place in the SuperSU ZIP installer, the deep sleep issue affects so many million users it is too big to ignore. (By the way, as far as I know this issue affects all recent high-end Samsungs).

While I don't disagree with your ideal of custom pre/post installers, in reality most users will never discover the issue, and just blame SuperSU for suddenly bad battery life. This leads to many support emails, thread posts, bad rep, etc.

Contrast this to for example the LG G3 compatibility patch, which requires the user to indeed write a file to /data or use a pre-installer that does that, the device will simply not boot, which forces the user to either go back to stock, or search for and discover the fix.

Either way, you are right, the patch doesn't even work right for Note users. Thank you for pointing that out - nobody else ever did. I have come up with the following improved script. If for the moment, we put aside our differences regarding the inclusion of any device-specific fixes, what do you think of the following?

It will perform the cache_type change for any scsi_disk, but will skip the ones not set to write protected. This should catch the problem with devices that have a different disk layout, and prevent accidental reduced I/O speed for devices that are not affected.

Note that it is my understanding that the write protection mode cannot be reset without a flash chip power cycle, and as the protection is set by the bootloader long before our check, checking once at boot should suffice.

I would be grateful if you gave that a shot on an affected Note/Edge+ and report back. It successfully sets the cache_type for :1 and :2 on my S6.

Code:

for i in `ls /sys/class/scsi_disk/`; do 
  cat /sys/class/scsi_disk/$i/write_protect 2>/dev/null | grep 1 >/dev/null
  if [ $? -eq 0 ]; then
    echo 'temporary none' > /sys/class/scsi_disk/$i/cache_type
  fi
done

garyd9 · Jan 12, 2016

Chainfire said:
I would be grateful if you gave that a shot on an affected Note/Edge+ and report back. It successfully sets the cache_type for :1 and :2 on my S6.

I won't be able to properly test this until at least tomorrow (Wed) evening... However, in the meantime, the following screenshots suggest that it'd also work on the Note5:

https://goo.gl/photos/61JWzoA5ir3PcDNr9

(This is with a custom kernel, however. I'll post a query in the Note 5 section asking people who are running a stock kernel to run similar commands to post the output here: http://xdaforums.com/showpost.php?p=64773152&postcount=138 - I'll relay the results.)

Let me know when you'd like to debate on if SuperSU should fix (non-root related) bugs in only specific devices, all devices, no devices, or if it should just support a hook to allow third parties to fix both current and future/past devices.

(Please don't get the wrong impression from that statement. SuperSU is your product, not mine... However you implement things is up to you.)

Chainfire · Jan 12, 2016

Please do let me know if I can be of further assistance to fix compatibility.

nkk71 said:
a) line 1170: dd if=/dev/zero of=$BOOTIMAGE bs=4096
since MultiROM creates a symlink, the above command actually starts nulling out a "dummy boot.img" file, which basically continues on, untill all free space in internal storage (or external sdcard where applicable) is filled out

I guess the script can be modified to detect a link and then check if said link is still pointing to /dev/...

Do double symlinks need to be taking into account? i.e. what is a symlink, /dev/block/platform/.../boot, /dev/block/mmcblk0pX, both?

b) when MultiROM-TWRP finishes installing SuperSU, the fake /data is still "busy" (some open file or something else keeping it busy), since it's busy, it can't be unmounted properly, and the real mount points don't get restored
at that point mrom injection will fail
using a lazy unmount helped alleviate that (as a workaround), but obviously not the best solution

Complete guesswork, but the backing file may need to be released for the loop device.

c) the setprop sukernel.mount 1 (in launch_daemonsu.sh) doesn't trigger the mount properly, workaround was to mount it in the launch_daemonsu.sh using "mount -t ext4 -o loop /data/su.img /su" instead of the setprop

Any idea why?

I'm specifically using the setprop / init.rc way because mount -o loop doesn't work on many firmwares.

d) the attempted remount read-only, will cause a bootloop; workaround: had to comment that out

Where is this?

nkk71 · Jan 12, 2016

Chainfire said:
Please do let me know if I can be of further assistance to fix compatibility.

Thank you, I will let you know once I've had a chance to properly debug further

I initially only wanted to get systemless root to work, which using the workarounds (even though not ideal), was proof it can be done

(at the time it was SuperSU v2.65)

Chainfire said:
I guess the script can be modified to detect a link and then check if said link is still pointing to /dev/...

Do double symlinks need to be taking into account? i.e. what is a symlink, /dev/block/platform/.../boot, /dev/block/mmcblk0pX, both?

No need to take double symlinks into account, only the real one is changed as follows:

Chainfire said:
Complete guesswork, but the backing file may need to be released for the loop device.

Will check, thanks.

Chainfire said:
Any idea why?

I'm specifically using the setprop / init.rc way because mount -o loop doesn't work on many firmwares.

Not really, everything else in init.rc get's executed properly; (and obviously the in launch_daemonsu.sh as well)

Chainfire said:
Where is this?

at the beginning of launch_daemonsu.sh:

Code:

if [ ! -d "/su/bin" ]; then
  # if we fstab'd system/vendor/oem to rw, remount them ro here
  remount_ro /system
  remount_ro /vendor
  remount_ro /oem

^^ I commented all three of them out, which worked out fine.
MultiROM's secondary ROMs always have system mounted rw, and the above remount_ro will force an immediate reboot

I need to do further testing on these issues, as soon as I come up with something more concrete, I will report back.

EDIT: forgot to mention, can confirm this for the HTC One M7, M8 and M9

garyd9 · Jan 13, 2016

garyd9 said:
I'll post a query in the Note 5 section asking people who are running a stock kernel to run similar commands to post the output here: http://xdaforums.com/showpost.php?p=64773152&postcount=138 - I'll relay the results.)

So, two replies. I've edited the results to just show the device and value of write_protect. The first one is a KNOX tripped device with completely stock firmware/kernel (no root):

Code:

scsi_disk/0:0:0:0    0
scsi_disk/0:0:0:1    1
scsi_disk/0:0:0:2    1
scsi_disk/0:0:0:3    1

The second is from a stock device where KNOX has never been tripped (the results are expected, but nice for confirmation):

Code:

scsi_disk/0:0:0:0    0
scsi_disk/0:0:0:1    0
scsi_disk/0:0:0:2    0
scsi_disk/0:0:0:3    0

So far, all indications are that the change suggested would work.

Can I have your permission to modify the superSU 2.66 archive to change the deepsleep script to use the script above and forward it to a couple people to validate? (Or, I can just wait until tomorrow night and do it on my own device.)

Chainfire · Jan 13, 2016

garyd9 said:
So far, all indications are that the change suggested would work.

Can I have your permission to modify the superSU 2.66 archive to change the deepsleep script to use the script above and forward it to a couple people to validate? (Or, I can just wait until tomorrow night and do it on my own device.)

Knock yourself out. I'm not in a rush though. I don't expect to release another update for another few days at least.

dr.ketan · Jan 13, 2016

Chainfire said:

Code:

for i in `ls /sys/class/scsi_disk/`; do 
  cat /sys/class/scsi_disk/$i/write_protect 2>/dev/null | grep 1 >/dev/null
  if [ $? -eq 0 ]; then
    echo 'temporary none' > /sys/class/scsi_disk/$i/cache_type
  fi
done

Confirmed working for all cache_types 1,2 and 3 but sorry I have patched kernel myself to fix so I have tested reverse just to prevent kernel swap.

Code:

echo 'write back' > /sys/class/scsi_disk/$i/cache_type

and it was write back for all 3. Indeed four including cache_type 0.0.0.0 as well)

So if i had test with

Code:

 echo 'temporary none' > /sys/class/scsi_disk/$i/cache_type

then 0000 also get cached right?
It shouldn't be problem right? Just references to this post last line.
Regards

Chainfire · Jan 13, 2016

dr.ketan said:
Confirmed working for all cache_types 1,2 and 3 but sorry I have patched kernel myself to fix so I have tested reverse just to prevent kernel swap.

Code:

echo 'write back' > /sys/class/scsi_disk/$i/cache_type

and it was write back for all 3. Indeed four including cache_type 0.0.0.0 as well)

So if i had test with

Code:

echo 'temporary none' > /sys/class/scsi_disk/$i/cache_type

then 0000 also get cached right?
It shouldn't be problem right? Just references to this post last line.
Regards

I don't know, since you changed it up, and your statement is a bit confusing.

Try this:

Code:

for i in `ls /sys/class/scsi_disk/`; do 
  cat /sys/class/scsi_disk/$i/write_protect 2>/dev/null | grep 1 >/dev/null
  if [ $? -eq 0 ]; then
    echo Write protected: $i
  else
    echo Write enabled: $i
  fi
done

Copy/paste the output.

dr.ketan · Jan 13, 2016

No problem. I will go to office in couple of hrs. Remove deep sleep fix from kernel and then test script as per said.

Chainfire · Jan 13, 2016

dr.ketan said:
No problem. I will go to office in couple of hrs. Remove deep sleep fix from kernel and then test script as per said.

That's not needed, just run the other script I pasted above. It'll show what we need to know regardless of your kernel being patched or not.

dr.ketan · Jan 13, 2016

u0_a259@noblelte:/ $ su
root@noblelte:/ # ls -l /sys/class/scsi_disk/
lrwxrwxrwx root root 2016-01-13 15:35 0:0:0:0 -> ../../devices/15570000.ufs/host0/target0:0:0/0:0:0:0/scsi_disk/0:0:0:0lrwxrwxrwx root root 2016-01-13 15:35 0:0:0:1 -> ../../devices/15570000.ufs/host0/target0:0:0/0:0:0:1/scsi_disk/0:0:0:1lrwxrwxrwx root root 2016-01-13 15:35 0:0:0:2 -> ../../devices/15570000.ufs/host0/target0:0:0/0:0:0:2/scsi_disk/0:0:0:2lrwxrwxrwx root root 2016-01-13 15:35 0:0:0:3 -> ../../devices/15570000.ufs/host0/target0:0:0/0:0:0:3/scsi_disk/0:0:0:3root@noblelte:/ # cat /sys/class/scsi_disk/*/write_protect
0
1
1
1
root@noblelte:/ #

Chainfire · Jan 13, 2016

dr.ketan said:
...

Seems fine!

nkk71 · Jan 13, 2016

I had some time to check a few things, so please find below my findings / possibly solutions

Chainfire said:
a) line 1170: dd if=/dev/zero of=$BOOTIMAGE bs=4096
since MultiROM creates a symlink, the above command actually starts nulling out a "dummy boot.img" file, which basically continues on, untill all free space in internal storage (or external sdcard where applicable) is filled out

Click to expand...

Click to collapse

I guess the script can be modified to detect a link and then check if said link is still pointing to /dev/...

Do double symlinks need to be taking into account? i.e. what is a symlink, /dev/block/platform/.../boot, /dev/block/mmcblk0pX, both?

Fixed it by using the following code (perhaps the readlink -f is redundant, but it worked fine)
(at line 1199 of SuperSU 2.66 updater-binary):

Code:

        if [ -b `readlink -f $BOOTIMAGE` ]; then
            dd if=/dev/zero of=$BOOTIMAGE bs=4096
        fi

Chainfire said:
b) when MultiROM-TWRP finishes installing SuperSU, the fake /data is still "busy" (some open file or something else keeping it busy), since it's busy, it can't be unmounted properly, and the real mount points don't get restored
at that point mrom injection will fail
using a lazy unmount helped alleviate that (as a workaround), but obviously not the best solution

Click to expand...

Click to collapse

Complete guesswork, but the backing file may need to be released for the loop device.

Turns out the loop device was in fact the problem; after umount /su, it still showed:

Code:

~ # [6n[B]losetup -a[/B]
losetup -a
/dev/block/loop0: 0 /data/su.img

so I just used/added (at line 1218 of SuperSU 2.66 updater-binary)

Code:

  umount /su
  [B]losetup -d /dev/block/loop0[/B]

I know it was on loop0 so I didnt need to account for anything else, but maybe using

Code:

LOOPDEVICE=`losetup -f`

or something similar, and continuing from there could be an option

Haven't tried checking on the other issues, but will report back when I have something on those

garyd9 · Jan 14, 2016

@Chainfire, early results on the deep sleep script change are 100% positive for both the Note5 and the edge+ devices.

Chainfire · Jan 14, 2016

nkk71 said:
I had some time to check a few things, so please find below my findings / possibly solutions

Thanks for the update. I'll have a look at those commands. losetup -f is specifically not used because I have already encountered devices/recoveries where the built-in losetup does not support this flag. So just loop0 and loop1 are tried, on failure, too bad (guess that could use improvement, hehe). The same goes for the -b test for if, and the -f flag for readlink. While I have not specifically tested the block device test, I know the symlink test fails on some devices. So I need to do some testing.

This is why some things in the ZIP are done is such weird ways instead of just using standard command, they're all work-arounds for encounted recovery versions that didn't support command X or flag Y ...

garyd9 said:
@Chainfire, early results on the deep sleep script change are 100% positive for both the Note5 and the edge+ devices.

Good news, as expected.

[DEVS-ONLY] SuperSU developer discussion

Moderator Emeritus / Senior Recognized Developer

Inactive Recognized Developer

Senior Member

Inactive Recognized Developer

Inactive Recognized Developer / Contributor

Moderator Emeritus / Senior Recognized Developer

Inactive Recognized Developer

Moderator Emeritus / Senior Recognized Developer

Inactive Recognized Developer / Contributor

Inactive Recognized Developer

Moderator Emeritus / Senior Recognized Developer

Recognized Developer / Recognized Contributor

Moderator Emeritus / Senior Recognized Developer

Recognized Developer / Recognized Contributor

Moderator Emeritus / Senior Recognized Developer

Recognized Developer / Recognized Contributor

Moderator Emeritus / Senior Recognized Developer

Inactive Recognized Developer / Contributor

Inactive Recognized Developer

Moderator Emeritus / Senior Recognized Developer

Similar threads

Top Liked Posts